diff --git a/.gitattributes b/.gitattributes index cf968721..056e3cb9 100644 --- a/.gitattributes +++ b/.gitattributes @@ -1 +1,2 @@ +* text=auto eol=lf package-lock.json linguist-generated=true diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index d1763cba..f7ddb49a 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -23,10 +23,10 @@ jobs: steps: - name: Checkout code - uses: actions/checkout@v6 + uses: actions/checkout@v7 - name: Setup Node.js - uses: actions/setup-node@v6 + uses: actions/setup-node@v7 with: node-version: '22.x' cache: 'npm' @@ -79,7 +79,7 @@ jobs: # smoke reuse the cache instead of downloading its own copy. The Storybook # step below reuses the same install. - name: Cache Playwright browsers - uses: actions/cache@v4 + uses: actions/cache@v6 with: path: ~/.cache/ms-playwright key: playwright-${{ runner.os }}-${{ hashFiles('clients/web/package-lock.json') }} @@ -129,10 +129,10 @@ jobs: id-token: write steps: - name: Checkout code - uses: actions/checkout@v6 + uses: actions/checkout@v7 - name: Setup Node.js - uses: actions/setup-node@v6 + uses: actions/setup-node@v7 with: node-version: '22.x' cache: 'npm' @@ -217,7 +217,7 @@ jobs: id-token: write steps: - name: Checkout code - uses: actions/checkout@v6 + uses: actions/checkout@v7 - name: Log in to the Container registry uses: docker/login-action@v4 @@ -241,14 +241,14 @@ jobs: latest=true - name: Set up QEMU - uses: docker/setup-qemu-action@v3 + uses: docker/setup-qemu-action@v4 - name: Set up Docker Buildx uses: docker/setup-buildx-action@v4 - name: Build and push Docker image id: push - uses: docker/build-push-action@v6 + uses: docker/build-push-action@v7 with: context: . push: true diff --git a/AGENTS.md b/AGENTS.md index 21061009..87d51230 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -17,7 +17,10 @@ v2/main/ │ │ │ # sandbox-controller.ts (MCP Apps sandbox HTTP server), │ │ │ # inject-auth-token.ts (embeds the API token into served index.html), │ │ │ # vite-base-config.ts (shared optimizeDeps exclusions), -│ │ │ # resolve-bind-host.ts (bind-host POLICY: refuses an +│ │ │ # resolve-bind-host.ts (bind-host POLICY: defaults to +│ │ │ # 127.0.0.1 — an ADDRESS, never the name `localhost`, which +│ │ │ # listen() resolves to a SINGLE family (::1 on glibc Linux), +│ │ │ # refusing every IPv4 client — #1951; and refuses an │ │ │ # all-interfaces HOST unless DANGEROUSLY_BIND_ALL_INTERFACES; │ │ │ # the all-interfaces DETECTION is core/node/hostUrl.isAllInterfacesHost. │ │ │ # Used by both bind points — web-server-config.ts + vite.config.ts — #1795), @@ -52,7 +55,15 @@ v2/main/ │ │ # annotation scan/validation + mirrored-param │ │ # derivation, used by the Tools tab — #1632; │ │ # plus `Mcp-Param-*` header building for the -│ │ # wire, used by both `tools/call` paths — #1846) +│ │ # wire, used by both `tools/call` paths — #1846; +│ │ # nullableUnion.ts: collapses the two JSON Schema +│ │ # encodings of "T or null" (`anyOf` with a null +│ │ # branch, `type: [T,"null"]`) into a plain type, +│ │ # hoisting the surviving branch's own keywords. +│ │ # Shared by BOTH form builders — web SchemaForm +│ │ # and TUI schemaToForm — since each dispatches on +│ │ # a single `type` string and would otherwise miss +│ │ # a nullable field entirely — #1928/#2015) │ ├── logging/ # Silent pino logger singleton │ ├── mcp/ # InspectorClient runtime + state stores │ │ # (modernTaskSchemas.ts: SEP-2663 modern Tasks @@ -462,6 +473,10 @@ Don't lean on GitHub's permission gate to enforce this. Whether an outside repor So v2 flows `feature branch → v2/main → (milestone) main → npm latest`, while v1 is flat: `feature branch → v1/main → npm v1-latest`, with no merge into `main` at any point. The two lines are published independently under separate dist-tags, which is why a v1 fix does **not** need to be forward-ported to reach users on v1 (`npx @modelcontextprotocol/inspector@v1-latest`). + ⚠️ **The version bump rides that same flow — it is made on `v2/main`, never on the milestone-merge branch (#2010).** `npm version --no-git-tag-version` lands on `v2/main` *before* the milestone merge, so the bump reaches `main` as part of the milestone's work; the release tag (bare `x.y.z`, no `v` prefix) is then applied to the resulting **`main`** commit. This deliberately leaves a normal in-flight interval where `v2/main` reads the version being built and `main` the one currently released — that difference is expected. The invariant it buys is that they converge when the release merge lands, so `v2/main` is never left *behind* `main` afterwards. Bumping on the milestone-merge branch instead (which is cut from `main`) puts the bump only downstream of `v2/main`, where nothing carries it back — that is how `v2/main` sat at `2.0.0` through both the 2.1.0 and 2.2.0 releases, and how #2009 came to carry an unrelated `2.0.0 → 2.2.0` diff simply because its branch was cut from a milestone-merge branch. **Never close a drift by merging `main` into `v2/main`**: `main` holds the whole pre-v2 v1 history (~230 commits `v2/main` lacks), so a back-merge grafts all of it into the develop branch permanently to deliver a two-file change. Full procedure in the root README's [Cutting a release](./README.md#cutting-a-release). + + A corollary for **branching**: cut feature branches from **`v2/main`**, never from a milestone-merge branch — the latter carries release-only commits that will show up in your PR's diff. + - **Project Boards**: - v2 - https://github.com/orgs/modelcontextprotocol/projects/28 (active board — all new work goes here) - v1 - https://github.com/orgs/modelcontextprotocol/projects/11 (legacy inspector version, no new activity except security fixes) diff --git a/Dockerfile b/Dockerfile index f87a32ce..24455398 100644 --- a/Dockerfile +++ b/Dockerfile @@ -31,8 +31,12 @@ RUN npm install -g /tmp/inspector.tgz && rm /tmp/inspector.tgz ENV HOST=0.0.0.0 \ DANGEROUSLY_BIND_ALL_INTERFACES=true \ CLIENT_PORT=6274 \ + MCP_SANDBOX_PORT=6275 \ MCP_AUTO_OPEN_ENABLED=false -EXPOSE 6274 +# 6275 is the MCP Apps sandbox, a second listener the browser reaches directly. +# It is only needed for the Apps tab, so it is EXPOSEd but publishing it is +# optional — see the Docker section of the root README. +EXPOSE 6274 6275 # Run as the non-root `node` user the base image ships. The inspector resolves # its runtime-state dir (default catalog, OAuth token storage) from `HOME` diff --git a/README.md b/README.md index 55459ad0..467a4fc0 100644 --- a/README.md +++ b/README.md @@ -32,7 +32,8 @@ inspector/ ├── core/ # Shared code consumed via the `@inspector/core` alias (no package.json) │ ├── auth/ # OAuth: providers, discovery, storage, mid-session recovery (browser/node/remote backends) │ ├── client/ # Install-level client config (`client.json`): browser-safe parse/validate + Node load/save, remote backend, secrets -│ ├── json/ # JSON + parameter/argument conversion utilities +│ ├── json/ # JSON + parameter/argument conversion utilities, and the nullable-union +│ │ # schema collapse shared by the web and TUI form builders │ ├── logging/ # Silent pino logger singleton │ ├── mcp/ # InspectorClient runtime, state stores, transports, config import │ ├── node/ # Node-only shared helpers: version reader, hostUrl (host normalize/canonicalize + all-interfaces/loopback detection) @@ -145,6 +146,7 @@ Each config below is a ready-made server for exercising one feature by hand. Loa | `pagination-http.json` | Page-by-page list fetching | [#1721](https://github.com/modelcontextprotocol/inspector/issues/1721) | | `structured-output-http.json` | Tools tab: a result's `structuredContent` section | [#1908](https://github.com/modelcontextprotocol/inspector/issues/1908) | | `duplicate-tool-names-http.json` | A `tools/list` that repeats a tool name | [#1957](https://github.com/modelcontextprotocol/inspector/issues/1957) | +| `nullable-fields-http.json` | Tools tab: nullable (`anyOf` + `null`) arguments | [#1928](https://github.com/modelcontextprotocol/inspector/issues/1928) | | `advertised-extensions-http.json` | Tool registration gated on advertised extensions | [#1739](https://github.com/modelcontextprotocol/inspector/issues/1739) | | `logging-{legacy,modern}-http.json` | Logging, both eras | [#1629](https://github.com/modelcontextprotocol/inspector/issues/1629) | | `subscriptions-{legacy,modern}-http.json` | Resource subscriptions, both eras | [#1630](https://github.com/modelcontextprotocol/inspector/issues/1630) | @@ -227,6 +229,14 @@ Connect (default legacy era), open the Tools tab, and type `get` into **Search t The duplicated copies are appended rather than placed beside their twin on purpose. React matches a leading run of same-key children first, so a head-adjacent duplicate happens to line up and the defect hides; separating the pair is what makes it observable — and it is also the realistic shape, two tool sources concatenated. +#### Nullable arguments + +`nullable-fields-http.json` serves `record_shipment`, whose four arguments are each declared with Zod's `.nullish()` — "optional **and** explicitly nullable". That compiles to `anyOf: [, { "type": "null" }]`, so the real type (and, for the enum, its `enum` list) sits on a branch rather than at the top level. `get_temp` sits alongside it with a plain, non-nullable `units` enum for comparison. Plain streamable-HTTP — connect with the **default (legacy)** protocol era. + +Open the Tools tab and select `record_shipment`: `direction` must render as a **Select** (`envio` / `recebimento`) with a clear button that sets it back to `null`, `reference` as a text input, `quantity` as a number input, and `express` as a checkbox. On the broken build every one of them fell through to the raw-JSON textarea, which re-escaped its own contents on each keystroke until the value was unusable ([#1928](https://github.com/modelcontextprotocol/inspector/issues/1928)). The tool echoes the arguments it received, so the result panel shows exactly what was sent. + +The **TUI** had the same gap and is worth checking against the same server (`--tui`, then test `record_shipment`): `direction` is a select, `quantity` an integer field, `express` a boolean. Both clients now share one collapse step — `normalizeNullableUnion` in [`core/json/nullableUnion.ts`](./core/json/nullableUnion.ts) — precisely so they cannot drift on which schemas they can render. + #### Advertised extensions `advertised-extensions-http.json` serves `echo` (always) and a `get_weather` tool **gated on the `io.modelcontextprotocol/tasks` extension** (`extensionGatedTools`): the tool is registered but starts disabled, and the server enables it on `notifications/initialized` only when the client declared that extension in its `capabilities.extensions`. @@ -326,16 +336,42 @@ Publishing is automated by two release-gated jobs in [`.github/workflows/main.ym A v2 release is cut from **`main`**, after the milestone's work has been merged there from `v2/main` — not from `v2/main` itself. (The v1 line releases independently from `v1/main` to the `v1-latest` tag and never touches `main`; see [Repo status](#mcp-inspector).) -Because there is **one version number** (only the root `package.json` has one — the clients carry none, so there is nothing to keep in sync and no `check-version` step), the release flow is just: +Because there is **one version number** (only the root `package.json` has one — the clients carry none, so there is nothing to keep in sync and no `check-version` step), the release flow is three steps. + +**1. Bump on `v2/main`, before the milestone merge.** The bump is part of the milestone's work, so it belongs on the develop branch and flows into `main` with everything else: + +Substitute the real issue number and release below — the commands are written to be copy-pasteable as-is (a `2.2.0` → `2.3.0` minor bump): ```bash -npm version # bumps the root package.json + tags -git push --follow-tags +git checkout -b v2/chore/2010-bump-2-3-0 v2/main +npm version minor --no-git-tag-version # or major / patch; bump only, no tag +# PR → v2/main +``` + +⚠️ **`--no-git-tag-version` is load-bearing.** A bare `npm version` also tags, and the tag would land on a `v2/main` commit — but the release must be cut from `main`, so the tag has to point at the merge commit there (step 3). Tagging here creates a tag on a commit that is never released. + +**2. Merge `v2/main` → `main`** through the usual milestone-merge branch. It now carries the bump, so the release lands on `main` with the version already correct. + +Between steps 1 and 2 the two branches **do** differ, and that is expected, not drift: `v2/main` reads the version being built while `main` still reads the one currently released. What this ordering removes is *post-release* drift — once the milestone merge lands they agree again, and `v2/main` is never left **behind** `main`. If you see `v2/main` ahead of `main`, a release is in flight; if you see it behind, something went wrong. + +**3. Tag the `main` commit and draft the Release:** + +```bash +git fetch origin main +git tag 2.3.0 origin/main && git push origin 2.3.0 # then draft & publish a GitHub Release for that tag → triggers `publish` ``` +⚠️ **Tag `origin/main`, not your local `HEAD`.** `git checkout main && git pull` resolves through whatever merge-or-rebase strategy you have configured, so a divergent local `main` can quietly produce or replay local commits. Tagging `HEAD` there tags a commit that is not on `origin/main`, and `git push origin ` pushes only the tag — leaving a release whose commit was never published. Naming `origin/main` explicitly makes the tagged commit exactly what the remote branch points at, regardless of local state. + +⚠️ **No `v` prefix.** This repo's release tags are bare `x.y.z` — `2.2.0`, `2.1.0`, `2.0.0` — so tag `2.3.0`, not `v2.3.0`. Note npm's own `tag-version-prefix` defaults to `v` and the repo sets no `.npmrc`, so a bare `npm version` would have produced a `v`-prefixed tag that does not match the convention. Tagging by hand (step 3) is what keeps it right. The workflow's assert step strips a leading `v` before comparing, so a `v`-prefixed tag would still publish — it would just be inconsistent with every previous release. + The release's target commit selects which workflow runs, so this only publishes when a release is cut from a commit carrying this (v2) workflow. +**Why the bump goes on `v2/main` first ([#2010](https://github.com/modelcontextprotocol/inspector/issues/2010)).** It used to happen on the milestone-merge branch, which is cut from `main` — so the bump existed only *downstream* of `v2/main` and nothing carried it back. `v2/main` sat at `2.0.0` through both the 2.1.0 and 2.2.0 releases. That is not cosmetic: a branch cut from a milestone-merge branch silently carries the bump into an unrelated PR (this happened on [#2009](https://github.com/modelcontextprotocol/inspector/issues/2009), where a container bugfix arrived with a `2.0.0 → 2.2.0` diff), and anything reading the version in development — `readInspectorVersion()`, `--version`, `GET /api/config` — reported a version two releases old. + +Do **not** "fix" a future drift by merging `main` back into `v2/main`. `main` carries the entire pre-v2 v1 history (retained through `ec5d8e13 chore: replace main's tree with v2` — ~230 commits `v2/main` does not have), so a back-merge grafts all of it into the develop branch's log permanently in order to deliver a two-file change. Bumping first means there is nothing to back-merge. + ### Docker A container image is published to GHCR (`ghcr.io/modelcontextprotocol/inspector`, `linux/amd64` + `linux/arm64`) by the release workflow. The [`Dockerfile`](Dockerfile) is a two-stage build: the first stage installs and `npm pack`s the publishable tarball; the second stage `npm install -g`s that tarball, so the image ships the exact same artifact as npm, with a clean `mcp-inspector` bin. @@ -349,6 +385,15 @@ docker build -t mcp-inspector . docker run --rm -p 127.0.0.1:6274:6274 mcp-inspector ``` +**Using the Apps tab? Publish `6275` too.** The MCP Apps sandbox is a second listener the browser reaches directly, on `MCP_SANDBOX_PORT` (default `6275`). Nothing else needs it, so the single-port commands above are fine for ordinary inspection — but the Apps tab renders a blank widget without it: + +```bash +docker run --rm -p 127.0.0.1:6274:6274 -p 127.0.0.1:6275:6275 \ + ghcr.io/modelcontextprotocol/inspector +``` + +Publish it on the **same port number** inside and out. The sandbox URL is handed to the browser via `/api/config` as `http://localhost:/sandbox`, so remapping it (`-p 9000:6275`) advertises a port the browser can't reach; use `-e MCP_SANDBOX_PORT=9000 -p 127.0.0.1:9000:9000` instead. + **Keep the `127.0.0.1:` prefix on the published port.** A bare `-p 6274:6274` publishes on **every host interface**, putting the Inspector on your local network. The container's `HOST=0.0.0.0` is a separate concern — it governs the _container's_ interfaces, not the host's — so the `DANGEROUSLY_BIND_ALL_INTERFACES` opt-in that guards a wildcard bind outside a container does not cover this. It matters more here than for an ordinary web app: the backend spawns processes on request, `GET /` embeds the API token into the served HTML, and a request arriving with **no** `Origin` header skips the origin allow-list entirely — so for any non-browser client the API token is the only guard. Publishing wider needs a real access-control boundary in front of the Inspector — a reverse proxy that authenticates, an SSH tunnel, a private network. Setting your own `MCP_INSPECTOR_API_TOKEN` does **not** substitute: `GET /` discloses whatever token is in use, so a custom one is harvested exactly as easily as a generated one. **Keeping the servers you add.** The Inspector saves your server list to `$HOME/.mcp-inspector/mcp.json`, which in the image is `/home/node/.mcp-inspector/mcp.json` — inside the container's writable layer, so `--rm` discards it and every run starts with an empty list. Mount a volume there to keep it: diff --git a/clients/tui/__tests__/schemaToForm.test.ts b/clients/tui/__tests__/schemaToForm.test.ts index 7efca937..f47450e3 100644 --- a/clients/tui/__tests__/schemaToForm.test.ts +++ b/clients/tui/__tests__/schemaToForm.test.ts @@ -213,4 +213,173 @@ describe("schemaToForm", () => { ); expect(form.sections[0]!.fields[0]).toMatchObject({ initialValue: "hi" }); }); + + // #2015 (the TUI twin of #1928): an argument declared with Zod's `.nullish()` + // has no top-level `type`/`enum` — they sit on the surviving `anyOf` branch — + // so before normalization every one of these degraded to a plain text field. + describe("nullable unions", () => { + it("renders a select for an anyOf string-enum|null argument", () => { + const form = schemaToForm( + { + properties: { + direction: { + anyOf: [ + { type: "string", enum: ["envio", "recebimento"] }, + { type: "null" }, + ], + }, + }, + }, + "nullableEnum", + ); + expect(form.sections[0]!.fields[0]).toMatchObject({ + name: "direction", + type: "select", + options: [ + { label: "envio", value: "envio" }, + { label: "recebimento", value: "recebimento" }, + ], + }); + }); + + it("maps the remaining nullable scalar branches to their typed fields", () => { + const form = schemaToForm( + { + properties: { + reference: { anyOf: [{ type: "string" }, { type: "null" }] }, + quantity: { + anyOf: [ + { type: "integer", minimum: 1, maximum: 9 }, + { type: "null" }, + ], + }, + ratio: { anyOf: [{ type: "number" }, { type: "null" }] }, + express: { anyOf: [{ type: "boolean" }, { type: "null" }] }, + }, + }, + "nullableScalars", + ); + const fields = form.sections[0]!.fields; + expect(fields.map((f) => ({ name: f.name, type: f.type }))).toEqual([ + { name: "reference", type: "string" }, + { name: "quantity", type: "integer" }, + { name: "ratio", type: "float" }, + { name: "express", type: "boolean" }, + ]); + // The branch's own constraints are hoisted along with its type. + expect(fields[1]).toMatchObject({ min: 1, max: 9 }); + }); + + it("renders a select for a nullable array-of-enum argument", () => { + const form = schemaToForm( + { + properties: { + tags: { + anyOf: [ + { type: "array", items: { enum: ["a", "b"] } }, + { type: "null" }, + ], + }, + }, + }, + "nullableArrayEnum", + ); + expect(form.sections[0]!.fields[0]).toMatchObject({ + type: "select", + options: [ + { label: "a", value: "a" }, + { label: "b", value: "b" }, + ], + }); + }); + + // With the null left in the list, the all-strings check would reject the + // enum and this would degrade to a plain text field. + it("renders a select for a type: [string, null] enum, null stripped", () => { + const form = schemaToForm( + { + properties: { + direction: { + type: ["string", "null"], + enum: ["envio", "recebimento", null], + }, + }, + }, + "typeArrayNullableEnum", + ); + expect(form.sections[0]!.fields[0]).toMatchObject({ + type: "select", + options: [ + { label: "envio", value: "envio" }, + { label: "recebimento", value: "recebimento" }, + ], + }); + }); + + it("collapses the type: [T, null] encoding too", () => { + const form = schemaToForm( + { + properties: { + note: { type: ["string", "null"] }, + flag: { type: ["boolean", "null"] }, + }, + }, + "typeArrayNull", + ); + expect( + form.sections[0]!.fields.map((f) => ({ name: f.name, type: f.type })), + ).toEqual([ + { name: "note", type: "string" }, + { name: "flag", type: "boolean" }, + ]); + }); + + // ink-form hands a select's stringified value straight back, so a numeric + // enum routed to a select would submit "1" for 1. The typed field loses the + // enum constraint but keeps the value's type — the safer loss. + it("routes a nullable numeric enum to its typed field, not a select", () => { + const form = schemaToForm( + { + properties: { + level: { + anyOf: [{ type: "integer", enum: [1, 2] }, { type: "null" }], + }, + ratio: { + anyOf: [{ type: "number", enum: [0.5, 1.5] }, { type: "null" }], + }, + }, + }, + "numericEnum", + ); + expect( + form.sections[0]!.fields.map((f) => ({ name: f.name, type: f.type })), + ).toEqual([ + { name: "level", type: "integer" }, + { name: "ratio", type: "float" }, + ]); + }); + + it("routes a plain numeric enum to its typed field too", () => { + const form = schemaToForm( + { properties: { level: { type: "integer", enum: [1, 2] } } }, + "plainNumericEnum", + ); + expect(form.sections[0]!.fields[0]).toMatchObject({ type: "integer" }); + }); + + it("leaves a union of two real types as a plain string field", () => { + const form = schemaToForm( + { + properties: { + mixed: { anyOf: [{ type: "string" }, { type: "number" }] }, + }, + }, + "realUnion", + ); + expect(form.sections[0]!.fields[0]).toMatchObject({ + name: "mixed", + type: "string", + }); + }); + }); }); diff --git a/clients/tui/src/utils/schemaToForm.ts b/clients/tui/src/utils/schemaToForm.ts index bbb730f2..6332450c 100644 --- a/clients/tui/src/utils/schemaToForm.ts +++ b/clients/tui/src/utils/schemaToForm.ts @@ -3,10 +3,19 @@ */ import type { FormStructure, FormSection, FormField } from "ink-form"; +import { + isStringEnum, + normalizeNullableUnion, +} from "@inspector/core/json/nullableUnion.js"; /** Minimal JSON Schema property shape used when building tool parameter forms */ interface JsonSchemaProperty { - type?: string; + /** + * An array here is the `["string", "null"]` nullable encoding, which + * {@link normalizeNullableUnion} collapses to a single name before the + * dispatch below reads it. + */ + type?: string | string[]; title?: string; enum?: unknown[]; /** Non-standard legacy support: titles for enum values */ @@ -15,6 +24,8 @@ interface JsonSchemaProperty { minimum?: number; maximum?: number; default?: unknown; + /** Present on a nullable union; see {@link normalizeNullableUnion}. */ + anyOf?: readonly unknown[]; } /** @@ -68,9 +79,16 @@ export function schemaToForm( // `properties` values are `unknown` (the SDK schema admits anything), so // guard before treating a value as a schema object — a malformed server // schema with e.g. `properties: { foo: null }` must not throw on `.title`. - const property = ( - typeof prop === "object" && prop !== null ? prop : {} - ) as JsonSchemaProperty; + // Flatten a nullable union (`anyOf: [X, {type:"null"}]`, `type: [X,"null"]`) + // before dispatching. Every branch below reads a single `type` string, and + // for an enum the `enum` keyword sits on the union's surviving branch — so + // without this an argument declared with Zod's `.nullish()` loses its + // select and degrades to a plain text field (#2015, the TUI twin of #1928). + const property = normalizeNullableUnion( + (typeof prop === "object" && prop !== null + ? prop + : {}) as JsonSchemaProperty, + ); const baseField = { name: key, label: property.title || key, @@ -90,12 +108,17 @@ export function schemaToForm( ...baseField, options: toSelectOptions(property.items.enum, property.items.enumNames), } as FormField; - } else if (property.enum) { - // Single select + } else if (isStringEnum(property.enum)) { + // Single select. Gated on the members being strings because + // `toSelectOptions` stringifies them and ink-form hands the string + // straight back: a numeric `enum: [1, 2]` would submit `"1"` and violate + // the schema. A typed non-string enum falls through to its typed field + // below, which loses the enum constraint but keeps the value's type — + // the safer of the two losses. field = { type: "select", ...baseField, - options: toSelectOptions(property.enum, property.enumNames), + options: toSelectOptions(property.enum ?? [], property.enumNames), } as FormField; } else { // Map JSON Schema types to ink-form types diff --git a/clients/web/README.md b/clients/web/README.md index a3facd92..01873854 100644 --- a/clients/web/README.md +++ b/clients/web/README.md @@ -162,7 +162,9 @@ The dev/prod backend guards every `/api/*` route with `x-mcp-remote-auth: Bearer ## Host binding & the origin allow-list -Both the prod backend (`server/web-server-config.ts`) and the dev Vite server (`vite.config.ts`) resolve their bind host through one shared guard, `server/resolve-bind-host.ts`. It binds `localhost` by default and **refuses an all-interfaces host** (`0.0.0.0`, `::`, empty, or any equivalent spelling — `0`, `0x0`, `0.0`, `::0`, `::ffff:0.0.0.0`, … are all folded to the wildcard and refused) — which would expose the process-spawning backend to the whole network, the exposure DNS-rebinding attacks target — unless `DANGEROUSLY_BIND_ALL_INTERFACES=true` is set. The Docker image sets that flag (a container must bind `0.0.0.0` to be reachable through `-p`); a bare `HOST=0.0.0.0` anywhere else exits with an actionable error. +Both the prod backend (`server/web-server-config.ts`) and the dev Vite server (`vite.config.ts`) resolve their bind host through one shared guard, `server/resolve-bind-host.ts`. It binds **`127.0.0.1`** by default and **refuses an all-interfaces host** (`0.0.0.0`, `::`, empty, or any equivalent spelling — `0`, `0x0`, `0.0`, `::0`, `::ffff:0.0.0.0`, … are all folded to the wildcard and refused) — which would expose the process-spawning backend to the whole network, the exposure DNS-rebinding attacks target — unless `DANGEROUSLY_BIND_ALL_INTERFACES=true` is set. The Docker image sets that flag (a container must bind `0.0.0.0` to be reachable through `-p`); a bare `HOST=0.0.0.0` anywhere else exits with an actionable error. + +**The default is the IPv4 loopback _address_, deliberately not the name `localhost`** (#1951). `server.listen(port, host)` resolves a name through `dns.lookup` and binds the **single** address it gets back; since Node 17 the default result order is `verbatim`, so on a glibc Linux host — whose `/etc/hosts` maps `localhost` to both `127.0.0.1` and `::1` — that first address is `::1`. `HOST=localhost` there binds IPv6 loopback **only** and refuses every IPv4 client. On a desktop this is invisible (the browser resolves `localhost` the same way and lands on `::1` too); it breaks wherever the connection is made by something that pins `127.0.0.1` — a VS Code dev container's port forwarder, `ssh -L 6274:127.0.0.1:6274`, a container healthcheck. Binding the address takes the resolver out of the decision. Browsing to `http://localhost:PORT` still works, since browsers fall back across address families and the loopback allow-list covers all three forms. An explicit `HOST=localhost` is still honored as typed — the single-family bind is then your choice. The backend's `/api/*` routes also enforce an **origin allow-list** (`allowedOrigins`) as DNS-rebinding protection. When left to default on a loopback host, it expands to all three interchangeable loopback origin forms for the port — `http://localhost:PORT`, `http://127.0.0.1:PORT`, and `http://[::1]:PORT` — because `localhost` resolves to either IPv4 or IPv6 loopback and Node/Vite may bind the IPv6 form, so the browser can legitimately arrive at `http://[::1]:PORT`. Set `ALLOWED_ORIGINS` (comma-separated) to override; entries are canonicalized (`new URL(o).origin`), so a trailing slash / uppercase host / explicit `:80` still match. **Each entry must include the scheme** — `http://localhost:6274`, not `localhost:6274` (a scheme-less value is dropped with a warning). `ALLOWED_ORIGINS` **replaces** the default list (it does not merge), so **list every origin you'll browse from, including the loopback forms** you still want (`http://localhost:PORT`, `http://127.0.0.1:PORT`, `http://[::1]:PORT`) — otherwise local access stops working. A blank `ALLOWED_ORIGINS` does **not** disable the check — it falls back to the default (fail closed); there is no env knob to turn origin validation off. @@ -176,7 +178,7 @@ The guard blocks only the **wildcard** all-interfaces addresses. Binding a **spe The bind-host guard and the `ALLOWED_ORIGINS` allow-list apply to both the prod server and `--dev`. Note that in **`--dev`** the Vite dev server _additionally_ enforces its own `server.allowedHosts` Host-header check, whose default accepts loopback and IP-literal hosts. The host you **bind** is auto-allowed (Vite adds the resolved `server.host` — which this config sets from `HOST` — to the allow-list), so `HOST=` works out of the box under `--dev` too. What needs an explicit `server.allowedHosts` entry is reaching the dev server at a **different** name than the one bound — e.g. a wildcard bind reached by hostname, or a reverse-proxy domain. For those, prefer the prod server (`mcp-inspector --web`) or add the host to `server.allowedHosts`. -**MCP Apps caveats.** The MCP Apps sandbox runs on a **separate** port (`MCP_SANDBOX_PORT`, dynamic by default). For the Apps tab to work off loopback, that sandbox port must be independently reachable from the browser — pin it with `MCP_SANDBOX_PORT` and expose/forward it too (the Docker image `EXPOSE`s only `6274`). (Under a `0.0.0.0` wildcard bind the sandbox URL is advertised as `localhost`, which is reachable — a wildcard bind serves loopback — so only the port needs handling.) Also note the sandbox iframe is gated by a `frame-ancestors` CSP, and **a bracketed IPv6 literal is not a valid CSP host-source** — so MCP Apps requires browsing the app at a name or IPv4 (`localhost`, `127.0.0.1`, a hostname, a LAN IPv4), **not** a bare `http://[::1]:…` address. Finally, the sandbox URL is always `http://` — so **behind TLS** (an `https://` app page) the browser blocks the `http://…/sandbox` iframe as mixed content and MCP Apps can't render; the Apps tab needs a plain-`http` app origin today. +**MCP Apps caveats.** The MCP Apps sandbox runs on a **separate** port — `MCP_SANDBOX_PORT`, defaulting to a fixed **`6275`** (#2008; it was OS-assigned before, which meant it changed every run and so could never be named in a `forwardPorts` / `-p` / tunnel config written ahead of time). For the Apps tab to work off loopback, that sandbox port must be independently reachable from the browser — expose/forward `6275` alongside `6274`, or set `MCP_SANDBOX_PORT` to pick another. If the port is already taken the sandbox falls back to an OS-assigned one and warns, so a second Inspector still gets a working Apps tab locally — but the forwarded port is then wrong, which is what the warning tells you. (Under a `0.0.0.0` wildcard bind the sandbox URL is advertised as `localhost`, which is reachable — a wildcard bind serves loopback — so only the port needs handling.) Also note the sandbox iframe is gated by a `frame-ancestors` CSP, and **a bracketed IPv6 literal is not a valid CSP host-source** — so MCP Apps requires browsing the app at a name or IPv4 (`localhost`, `127.0.0.1`, a hostname, a LAN IPv4), **not** a bare `http://[::1]:…` address. Finally, the sandbox URL is always `http://` — so **behind TLS** (an `https://` app page) the browser blocks the `http://…/sandbox` iframe as mixed content and MCP Apps can't render; the Apps tab needs a plain-`http` app origin today. In every case, exposing the Inspector beyond loopback also means anyone who can reach it can drive its backend — keep authentication on (do **not** set `DANGEROUSLY_OMIT_AUTH`) and prefer a specific bind address over the wildcard. diff --git a/clients/web/server/resolve-bind-host.ts b/clients/web/server/resolve-bind-host.ts index a6d6fda1..f18945ce 100644 --- a/clients/web/server/resolve-bind-host.ts +++ b/clients/web/server/resolve-bind-host.ts @@ -26,21 +26,46 @@ function isEnabled(value: string | undefined): boolean { return v === "true" || v === "1"; } +/** + * The default bind host: the IPv4 loopback **address**, deliberately not the + * name `localhost`. + * + * `server.listen(port, host)` resolves a name through `dns.lookup` and binds the + * **single** address it gets back. Since Node 17 the default result order is + * `verbatim`, so Node takes whatever the resolver returns first rather than + * preferring IPv4 — and on a glibc Linux host with the stock `/etc/hosts` + * (`127.0.0.1 localhost` *and* `::1 localhost`) that is `::1`. `HOST=localhost` + * therefore binds IPv6 loopback **only**, and every IPv4 client is refused. + * + * That is invisible on a desktop, where the browser resolves `localhost` the + * same way and lands on `::1` too. It breaks wherever the connection is made by + * something that pins IPv4 `127.0.0.1` instead — a VS Code dev container's port + * forwarder (#1951), an `ssh -L …:127.0.0.1:…` tunnel, a container healthcheck. + * Binding the address directly removes the resolver from the decision, and the + * loopback origin allow-list already covers `localhost`, `127.0.0.1`, and + * `[::1]` alike, so browsing to `http://localhost:PORT` still works (browsers + * fall back across address families; a Node `listen` does not). + */ +export const DEFAULT_BIND_HOST = "127.0.0.1"; + /** * Resolve the bind hostname from `env` (default `process.env`), defaulting to - * `localhost`. Refuses an all-interfaces host (`0.0.0.0` / `::` / empty / their - * legacy spellings) unless {@link BIND_ALL_INTERFACES_ENV} is explicitly - * enabled — the published Docker image sets it, since a container must bind - * `0.0.0.0` to be reachable through `-p`. Throws (fail fast, loudly) rather than - * silently binding wide open. The returned value is trimmed and de-bracketed - * (an IPv6 literal is returned bare, e.g. `HOST=[::1]` → `::1`) so detection, - * `listen()`, and the origin list all consume the same value; `formatHostForUrl` - * re-adds the brackets wherever a URL is built. + * {@link DEFAULT_BIND_HOST}. Refuses an all-interfaces host (`0.0.0.0` / `::` / + * empty / their legacy spellings) unless {@link BIND_ALL_INTERFACES_ENV} is + * explicitly enabled — the published Docker image sets it, since a container + * must bind `0.0.0.0` to be reachable through `-p`. Throws (fail fast, loudly) + * rather than silently binding wide open. The returned value is trimmed and + * de-bracketed (an IPv6 literal is returned bare, e.g. `HOST=[::1]` → `::1`) so + * detection, `listen()`, and the origin list all consume the same value; + * `formatHostForUrl` re-adds the brackets wherever a URL is built. + * + * An explicit `HOST=localhost` is honored as typed — the single-family bind + * described on {@link DEFAULT_BIND_HOST} is then the caller's choice, not ours. */ export function resolveBindHostname( env: NodeJS.ProcessEnv = process.env, ): string { - const host = (env.HOST ?? "localhost").trim(); + const host = (env.HOST ?? DEFAULT_BIND_HOST).trim(); if (isAllInterfacesHost(host) && !isEnabled(env[BIND_ALL_INTERFACES_ENV])) { // Show the resolved address when it differs from the typed spelling — the // guard now catches forms the resolver folds to the wildcard (a fullwidth diff --git a/clients/web/server/sandbox-controller.ts b/clients/web/server/sandbox-controller.ts index e4b0d53e..50d4c440 100644 --- a/clients/web/server/sandbox-controller.ts +++ b/clients/web/server/sandbox-controller.ts @@ -11,13 +11,14 @@ import { canonicalUrlHost, isAllInterfacesHost, } from "../../../core/node/hostUrl.ts"; +import { DEFAULT_BIND_HOST } from "./resolve-bind-host.js"; const __dirname = dirname(fileURLToPath(import.meta.url)); export interface SandboxControllerOptions { /** Port to bind (0 = dynamic). */ port: number; - /** Host to bind (default localhost). */ + /** Host to bind (default {@link DEFAULT_BIND_HOST}). */ host?: string; /** * The backend's origin allow-list (the embedder origins). Used to build the @@ -95,10 +96,28 @@ function parseListenPort(raw: string | undefined): number | undefined { } /** - * Resolve sandbox port from env: MCP_SANDBOX_PORT → SERVER_PORT → 0 (dynamic). - * An invalid value falls through rather than crashing the boot; a set-but-invalid - * MCP_SANDBOX_PORT (the dedicated knob) is warned so the fall-through isn't - * silent — matching the warn-and-drop precedent for ALLOWED_ORIGINS. + * The default sandbox listen port — **fixed**, deliberately not `0`. + * + * A `0` default lets the OS assign, which is fine on a desktop (the browser + * learns the port from `/api/config`) but unusable anywhere the port has to be + * declared *before* the process starts: a dev container's `forwardPorts`, a + * `docker run -p`, a k8s service, an SSH tunnel. The port changed on every run, + * so it could never be named in that config and the Apps tab was unreachable + * (#2008). `CLIENT_PORT` has always defaulted to a fixed `6274` for the same + * reason; this is its sandbox counterpart. + * + * `6275` sits next to the web port so the pair is easy to forward together. It + * is not a v1 carry-over — v2 has no proxy server on it. + */ +export const DEFAULT_SANDBOX_PORT = 6275; + +/** + * Resolve sandbox port from env: MCP_SANDBOX_PORT → SERVER_PORT → + * {@link DEFAULT_SANDBOX_PORT}. An invalid value falls through rather than + * crashing the boot; a set-but-invalid MCP_SANDBOX_PORT (the dedicated knob) is + * warned so the fall-through isn't silent — matching the warn-and-drop + * precedent for ALLOWED_ORIGINS. An explicit `0` is still honored as "let the + * OS assign" ({@link parseListenPort} accepts it). */ export function resolveSandboxPort(): number { const fromSandbox = parseListenPort(process.env.MCP_SANDBOX_PORT); @@ -107,13 +126,21 @@ export function resolveSandboxPort(): number { `Ignoring invalid MCP_SANDBOX_PORT="${process.env.MCP_SANDBOX_PORT}" (need an integer 0–65535); falling back.`, ); } - return fromSandbox ?? parseListenPort(process.env.SERVER_PORT) ?? 0; + return ( + fromSandbox ?? + parseListenPort(process.env.SERVER_PORT) ?? + DEFAULT_SANDBOX_PORT + ); } export function createSandboxController( options: SandboxControllerOptions, ): SandboxController { - const { port, host = "localhost", allowedOrigins } = options; + // Defaulted to the same address the web server binds, never the name + // `localhost` — a name resolves to one address family and would reintroduce + // the #1951 split (web on IPv4, sandbox on IPv6) for any future call site + // that omits `host`. Both call sites pass `config.sandboxHost` today. + const { port, host = DEFAULT_BIND_HOST, allowedOrigins } = options; let server: Server | null = null; let sandboxUrl: string | null = null; @@ -177,8 +204,28 @@ export function createSandboxController( }); res.end(sandboxHtml); }); + // A taken port used to mean "no sandbox at all", which was tolerable + // only because the default was dynamic and therefore never collided. + // Now that it's fixed (see DEFAULT_SANDBOX_PORT), a second Inspector — + // or anything else already on 6275 — would take the Apps tab down, so + // retry once on an OS-assigned port. The retry is announced loudly: + // whoever pinned MCP_SANDBOX_PORT to forward it needs to know the pin + // didn't take, and a silently-moved port is exactly the failure this + // default was introduced to fix. + let retriedDynamic = false; server.on("error", (err: NodeJS.ErrnoException) => { if (err.code === "EADDRINUSE") { + if (!retriedDynamic && port !== 0) { + retriedDynamic = true; + console.warn( + `Sandbox: port ${port} in use; falling back to an OS-assigned port. ` + + `The MCP Apps tab will work locally, but the sandbox is no longer ` + + `on a predictable port — set MCP_SANDBOX_PORT to a free one if you ` + + `need to forward it (container / SSH tunnel / reverse proxy).`, + ); + server!.listen(0, host); + return; + } console.error( `Sandbox: port ${port || "dynamic"} in use. MCP Apps tab may not work.`, ); diff --git a/clients/web/src/components/groups/AppDetailPanel/AppDetailPanel.test.tsx b/clients/web/src/components/groups/AppDetailPanel/AppDetailPanel.test.tsx index effbd5a2..ad14b1c3 100644 --- a/clients/web/src/components/groups/AppDetailPanel/AppDetailPanel.test.tsx +++ b/clients/web/src/components/groups/AppDetailPanel/AppDetailPanel.test.tsx @@ -173,4 +173,30 @@ describe("AppDetailPanel", () => { rerender(); expect(input().value).toBe(""); }); + + // #2020: `hasMissingRequiredFields` sees a field with no value, but text a + // field could not turn into a value looks identical to it — both report + // `undefined` — so an optional argument was dropped from the call with no + // gate. The form reports draft validity for this reason. + it("disables Open App while a field holds text it cannot send", async () => { + const user = userEvent.setup(); + const jsonFieldTool: Tool = { + name: "chart_app", + title: "Chart App", + inputSchema: { + type: "object", + properties: { series: { type: "array", title: "Series" } }, + }, + }; + renderWithMantine(); + const openApp = screen.getByRole("button", { name: /open app/i }); + expect(openApp).not.toBeDisabled(); + + const jsonInput = screen.getByLabelText(/Series/); + await user.type(jsonInput, "x"); + expect(openApp).toBeDisabled(); + + await user.clear(jsonInput); + expect(openApp).not.toBeDisabled(); + }); }); diff --git a/clients/web/src/components/groups/AppDetailPanel/AppDetailPanel.tsx b/clients/web/src/components/groups/AppDetailPanel/AppDetailPanel.tsx index 8d5a2135..8d75f431 100644 --- a/clients/web/src/components/groups/AppDetailPanel/AppDetailPanel.tsx +++ b/clients/web/src/components/groups/AppDetailPanel/AppDetailPanel.tsx @@ -1,4 +1,5 @@ import { Button, Divider, ScrollArea, Stack, Text } from "@mantine/core"; +import { useState } from "react"; import { MdPlayArrow } from "react-icons/md"; import type { Tool } from "@modelcontextprotocol/client"; import { SchemaForm } from "../SchemaForm/SchemaForm"; @@ -58,7 +59,12 @@ export function AppDetailPanel({ /* v8 ignore next -- unreachable: Tool.inputSchema is always an object */ const formSchema = toFormSchema(inputSchema) ?? {}; const hasErrors = hasMissingRequiredFields(formSchema, formValues); - const disabled = isOpening || hasErrors; + // A field holding text it could not turn into a value (unparseable JSON, an + // unrepresentable number) reports `undefined`, which is indistinguishable + // from empty once it reaches `formValues` — so the form reports it directly + // and an optional argument can no longer be dropped silently (#2020). + const [hasInvalidDraft, setHasInvalidDraft] = useState(false); + const disabled = isOpening || hasErrors || hasInvalidDraft; const hasFields = hasInputFields(tool); return ( @@ -82,6 +88,7 @@ export function AppDetailPanel({ // tool's name to drop another app's in-progress field text. See // SchemaFormProps.resetKey. resetKey={tool.name} + onValidityChange={setHasInvalidDraft} /> { await user.type(screen.getByLabelText("Host"), "l"); expect(onChange).toHaveBeenCalledWith({ host: "l" }); }); + + // #2020: text a field cannot turn into a value reports `undefined`, so it is + // indistinguishable from an empty field in `values` — Submit stayed enabled + // and the answer went out without the property. + it("disables Submit while a field holds text it cannot send", async () => { + const user = userEvent.setup(); + const numberRequest: ElicitRequestFormParams = { + message: "Please provide the port.", + requestedSchema: { + type: "object" as const, + properties: { port: { type: "number" as const, title: "Port number" } }, + }, + }; + renderWithMantine( + , + ); + const submit = screen.getByRole("button", { name: "Submit" }); + expect(submit).toBeEnabled(); + + // Past MAX_SAFE_INTEGER the field reports no value rather than a rounded + // one, so the answer would go out without the property. + const input = screen.getByLabelText(/Port number/); + await user.type(input, "90071992547409910"); + expect(submit).toBeDisabled(); + + await user.clear(input); + expect(submit).toBeEnabled(); + }); }); diff --git a/clients/web/src/components/groups/ElicitationFormPanel/ElicitationFormPanel.tsx b/clients/web/src/components/groups/ElicitationFormPanel/ElicitationFormPanel.tsx index 5ec4fddc..07802433 100644 --- a/clients/web/src/components/groups/ElicitationFormPanel/ElicitationFormPanel.tsx +++ b/clients/web/src/components/groups/ElicitationFormPanel/ElicitationFormPanel.tsx @@ -7,6 +7,7 @@ import { Stack, Text, } from "@mantine/core"; +import { useState } from "react"; import type { ElicitRequestFormParams } from "@modelcontextprotocol/client"; import type { PendingRequestOrigin } from "@inspector/core/mcp/types.js"; import { @@ -88,8 +89,13 @@ export function ElicitationFormPanel({ busy = false, }: ElicitationFormPanelProps) { const requestedSchema = request.requestedSchema as InspectorFormSchema; + // Text a field could not turn into a value never reaches `values`, so the + // form has to report it for Submit to see it at all (#2020). + const [hasInvalidDraft, setHasInvalidDraft] = useState(false); const submitDisabled = - busy || hasMissingRequiredFields(requestedSchema, values); + busy || + hasMissingRequiredFields(requestedSchema, values) || + hasInvalidDraft; return ( {formatQuoted(request.message)} @@ -101,6 +107,7 @@ export function ElicitationFormPanel({ values={values} onChange={onChange} disabled={busy} + onValidityChange={setHasInvalidDraft} /> {formatWarning(serverName)} diff --git a/clients/web/src/components/groups/InlineElicitationRequest/InlineElicitationRequest.test.tsx b/clients/web/src/components/groups/InlineElicitationRequest/InlineElicitationRequest.test.tsx index ea065596..b988e2b0 100644 --- a/clients/web/src/components/groups/InlineElicitationRequest/InlineElicitationRequest.test.tsx +++ b/clients/web/src/components/groups/InlineElicitationRequest/InlineElicitationRequest.test.tsx @@ -161,4 +161,70 @@ describe("InlineElicitationRequest", () => { ); expect(screen.getByText("Host")).toBeInTheDocument(); }); + + // #2020: this inline form had no submit gate at all — neither the missing + // required answer that `ElicitationFormPanel` already checked, nor text a + // field could not turn into a value (which never reaches `values`). + describe("submit gating (#2020)", () => { + const requiredRequest = { + message: "Please provide your name.", + requestedSchema: { + type: "object" as const, + properties: { name: { type: "string" as const, title: "Name" } }, + required: ["name"], + }, + } satisfies ElicitRequest["params"]; + + const numberRequest = { + message: "Please provide the port.", + requestedSchema: { + type: "object" as const, + properties: { port: { type: "number" as const, title: "Port number" } }, + }, + } satisfies ElicitRequest["params"]; + + it("disables Submit while a required answer is missing", () => { + renderWithMantine( + , + ); + expect(screen.getByRole("button", { name: "Submit" })).toBeDisabled(); + }); + + it("enables Submit once the required answer is given", () => { + renderWithMantine( + , + ); + expect(screen.getByRole("button", { name: "Submit" })).toBeEnabled(); + }); + + it("disables Submit while a field holds text it cannot send", async () => { + const user = userEvent.setup(); + renderWithMantine( + , + ); + const submit = screen.getByRole("button", { name: "Submit" }); + expect(submit).toBeEnabled(); + + // Past MAX_SAFE_INTEGER the field reports no value rather than a rounded + // one, so the answer would go out without the property. + const input = screen.getByLabelText(/Port number/); + await user.type(input, "90071992547409910"); + expect(submit).toBeDisabled(); + + await user.clear(input); + expect(submit).toBeEnabled(); + }); + }); }); diff --git a/clients/web/src/components/groups/InlineElicitationRequest/InlineElicitationRequest.tsx b/clients/web/src/components/groups/InlineElicitationRequest/InlineElicitationRequest.tsx index 14bc6fe8..7a85e6d2 100644 --- a/clients/web/src/components/groups/InlineElicitationRequest/InlineElicitationRequest.tsx +++ b/clients/web/src/components/groups/InlineElicitationRequest/InlineElicitationRequest.tsx @@ -13,7 +13,11 @@ import type { ElicitRequestFormParams, } from "@modelcontextprotocol/client"; import type { PendingRequestOrigin } from "@inspector/core/mcp/types.js"; -import type { InspectorFormSchema } from "../../../utils/jsonUtils"; +import { useState } from "react"; +import { + hasMissingRequiredFields, + type InspectorFormSchema, +} from "../../../utils/jsonUtils"; import { SchemaForm } from "../SchemaForm/SchemaForm"; import { MrtrOriginNote } from "../../elements/MrtrOriginNote/MrtrOriginNote"; @@ -86,6 +90,19 @@ export function InlineElicitationRequest({ onSubmit, onCancel, }: InlineElicitationRequestProps) { + // Same gate as `ElicitationFormPanel`, which this is the inline form of: a + // missing required answer, or a field holding text it could not turn into a + // value, must not be submittable — neither is visible in `values` (#2020). + const [hasInvalidDraft, setHasInvalidDraft] = useState(false); + const formValues = values ?? {}; + const submitDisabled = + isFormMode(request) && + (hasMissingRequiredFields( + request.requestedSchema as InspectorFormSchema, + formValues, + ) || + hasInvalidDraft); + return ( @@ -100,8 +117,9 @@ export function InlineElicitationRequest({ {isFormMode(request) && ( )} @@ -120,7 +138,7 @@ export function InlineElicitationRequest({ Cancel {isFormMode(request) && ( - )} diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx index 5b9f064a..6c7dd77b 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx @@ -2,6 +2,7 @@ import { useState } from "react"; import { describe, it, expect, vi } from "vitest"; import userEvent from "@testing-library/user-event"; import type { InspectorFormSchema } from "../../../utils/jsonUtils"; +import { toFormSchema } from "../../../utils/jsonUtils"; import { fireEvent, renderWithMantine, @@ -737,7 +738,52 @@ describe("SchemaForm", () => { expect(lastCall.config).toEqual([1, 2]); }); - it("falls back to passing raw string to onChange when JSON is invalid in JsonInput", async () => { + // The JSON field used to store unparseable text back as the *value*, which + // the next render re-stringified — so each keystroke added a layer of + // escaping (`[` → `"["` → `"\"[\""`). That compounding escape is #1928's + // original symptom, and it lived here rather than in the dispatch. It matters + // more since #2007, whose fix deliberately routes object unions to this + // editor: a fallback nobody can type into is not a fallback. + it("shows an error while the JSON draft does not parse", async () => { + const user = userEvent.setup(); + const schema: InspectorFormSchema = { + type: "object", + properties: { + config: { type: "array", title: "Config" }, + }, + }; + function Harness() { + const [values, setValues] = useState>({}); + return ( + + ); + } + renderWithMantine(); + + const jsonInput = screen.getByLabelText(/Config/) as HTMLTextAreaElement; + await user.type(jsonInput, "[[1,"); + // Invalid text yields no value, so without this the field would submit as + // absent while the user is still looking at what they typed. + expect(screen.getByText(/Not valid JSON/)).toBeInTheDocument(); + + await user.type(jsonInput, "2]"); + expect(screen.queryByText(/Not valid JSON/)).not.toBeInTheDocument(); + }); + + it("shows no error for an empty optional field", () => { + const schema: InspectorFormSchema = { + type: "object", + properties: { + config: { type: "array", title: "Config" }, + }, + }; + renderWithMantine( + , + ); + expect(screen.queryByText(/Not valid JSON/)).not.toBeInTheDocument(); + }); + + it("reports no value, not raw text, while the JSON is mid-edit", async () => { const user = userEvent.setup(); const onChange = vi.fn(); const schema: InspectorFormSchema = { @@ -751,9 +797,59 @@ describe("SchemaForm", () => { ); const jsonInput = screen.getByLabelText(/Config/) as HTMLTextAreaElement; await user.type(jsonInput, "x"); - expect(onChange).toHaveBeenCalled(); - const lastCall = onChange.mock.calls[onChange.mock.calls.length - 1][0]; - expect(typeof lastCall.config).toBe("string"); + expect(onChange).toHaveBeenLastCalledWith({ config: undefined }); + }); + + it("lets an array literal be typed one character at a time", async () => { + const user = userEvent.setup(); + const schema: InspectorFormSchema = { + type: "object", + properties: { + config: { type: "array", title: "Config" }, + }, + }; + // Drive the real controlled loop: each onChange feeds straight back in as + // `values`, which is what turned the old handler's raw-text write into a + // compounding re-escape. + function Harness() { + const [values, setValues] = useState>({}); + return ( + + ); + } + renderWithMantine(); + + const jsonInput = screen.getByLabelText(/Config/) as HTMLTextAreaElement; + // `[` is a userEvent keyboard descriptor, so it is escaped as `[[`. + await user.type(jsonInput, '[[1,"a"]'); + + // The box shows exactly what was typed — no injected quotes or backslashes. + expect(jsonInput.value).toBe('[1,"a"]'); + }); + + it("keeps an in-progress draft visible instead of rewriting it", async () => { + const user = userEvent.setup(); + const schema: InspectorFormSchema = { + type: "object", + properties: { + config: { type: "array", title: "Config" }, + }, + }; + function Harness() { + const [values, setValues] = useState>({}); + return ( + + ); + } + renderWithMantine(); + + const jsonInput = screen.getByLabelText(/Config/) as HTMLTextAreaElement; + await user.type(jsonInput, "[[1,"); + // Unparseable so far, and it must survive the re-render untouched. + expect(jsonInput.value).toBe("[1,"); + + await user.type(jsonInput, "2]"); + expect(jsonInput.value).toBe("[1,2]"); }); it("uses default values when value is undefined", () => { @@ -811,3 +907,573 @@ describe("SchemaForm", () => { expect(container.firstChild).not.toBeNull(); }); }); + +// #1928: "optional AND explicitly nullable" (Zod's `.nullish()`) compiles to a +// nullable union rather than a plain type. Before the normalization step these +// matched no branch and fell through to the raw-JSON fallback, where every +// keystroke re-escaped the value. +describe("SchemaForm nullable unions", () => { + it("renders a Select for an anyOf string-enum|null field", async () => { + const user = userEvent.setup(); + const onChange = vi.fn(); + const schema: InspectorFormSchema = { + type: "object", + properties: { + type: { + title: "Type", + anyOf: [ + { type: "string", enum: ["envio", "recebimento"] }, + { type: "null" }, + ], + }, + }, + }; + renderWithMantine( + , + ); + await user.click(screen.getByRole("textbox", { name: "Type" })); + await user.click( + await screen.findByRole("option", { name: "envio", hidden: true }), + ); + expect(onChange).toHaveBeenCalledWith({ type: "envio" }); + }); + + it("clears a nullable enum back to null", async () => { + const user = userEvent.setup(); + const onChange = vi.fn(); + const schema: InspectorFormSchema = { + type: "object", + properties: { + type: { + title: "Type", + anyOf: [{ type: "string", enum: ["envio"] }, { type: "null" }], + }, + }, + }; + renderWithMantine( + , + ); + // Mantine marks its combobox clear button `aria-hidden` (it is mouse-only, + // `tabIndex={-1}`), so it is only reachable with `hidden: true`. + await user.click(screen.getByRole("button", { hidden: true })); + expect(onChange).toHaveBeenCalledWith({ type: null }); + }); + + it("offers no clear affordance on a non-nullable enum", () => { + const schema: InspectorFormSchema = { + type: "object", + properties: { + format: { type: "string", title: "Format", enum: ["json"] }, + }, + }; + renderWithMantine( + , + ); + expect( + screen.queryByRole("button", { hidden: true }), + ).not.toBeInTheDocument(); + }); + + // The other supported nullable encoding: keywords stay at the top level, so + // the null sentinel sits inside the enum list rather than on a branch. + it("renders a Select for a type: [string, null] enum, without a null option", async () => { + const user = userEvent.setup(); + const onChange = vi.fn(); + // Built through `toFormSchema`, the same narrowing boundary every + // production call site uses, rather than cast into `InspectorFormSchema`. + // A `null` member is valid JSON Schema but outside that type's `string[]` + // `enum`, and this fixture *is* wire data — so the honest way to introduce + // it is the wire→form narrow, not a double cast that erases the mismatch. + const schema = toFormSchema({ + type: "object", + properties: { + direction: { + title: "Direction", + type: ["string", "null"], + enum: ["envio", "recebimento", null], + }, + }, + }); + renderWithMantine( + , + ); + await user.click(screen.getByRole("textbox", { name: "Direction" })); + const options = await screen.findAllByRole("option", { hidden: true }); + expect(options.map((option) => option.textContent)).toEqual([ + "envio", + "recebimento", + ]); + await user.click(options[0]); + expect(onChange).toHaveBeenCalledWith({ direction: "envio" }); + }); + + it("renders a TextInput for a type: [string, null] field", async () => { + const user = userEvent.setup(); + const onChange = vi.fn(); + const schema: InspectorFormSchema = { + type: "object", + properties: { + note: { type: ["string", "null"], title: "Note" }, + }, + }; + renderWithMantine( + , + ); + await user.type(screen.getByLabelText(/Note/), "a"); + expect(onChange).toHaveBeenCalledWith({ note: "a" }); + }); + + it("renders a checkbox for an anyOf boolean|null field", async () => { + const user = userEvent.setup(); + const onChange = vi.fn(); + const schema: InspectorFormSchema = { + type: "object", + properties: { + enabled: { + title: "Enabled", + anyOf: [{ type: "boolean" }, { type: "null" }], + }, + }, + }; + renderWithMantine( + , + ); + await user.click(screen.getByLabelText("Enabled")); + expect(onChange).toHaveBeenCalledWith({ enabled: true }); + }); + + it("renders a MultiSelect for an anyOf array-of-enum|null field", async () => { + const user = userEvent.setup(); + const onChange = vi.fn(); + const schema: InspectorFormSchema = { + type: "object", + properties: { + tags: { + title: "Tags", + anyOf: [ + { type: "array", items: { type: "string", enum: ["a", "b"] } }, + { type: "null" }, + ], + }, + }, + }; + renderWithMantine( + , + ); + await user.click(screen.getByRole("textbox", { name: "Tags" })); + await user.click( + await screen.findByRole("option", { name: "a", hidden: true }), + ); + expect(onChange).toHaveBeenCalledWith({ tags: ["a"] }); + }); + + it("renders nested fields for an anyOf object|null field", async () => { + const user = userEvent.setup(); + const onChange = vi.fn(); + const schema: InspectorFormSchema = { + type: "object", + properties: { + profile: { + title: "Profile", + anyOf: [ + { + type: "object", + properties: { nick: { type: "string", title: "Nick" } }, + }, + { type: "null" }, + ], + }, + }, + }; + renderWithMantine( + , + ); + await user.type(screen.getByLabelText(/Nick/), "z"); + expect(onChange).toHaveBeenCalledWith({ profile: { nick: "z" } }); + }); + + // #2007: `z.array(z.union([z.object(…), z.object(…)]))` gives an `items.anyOf` + // whose branches carry no top-level `const`, so every MultiSelect option was + // the empty string — and Mantine *throws* on duplicate option values, greying + // out the whole tool panel. The nullable form below is reachable only because + // this PR now collapses it into `type: "array"`, so it has to be safe too. + it("falls back to the JSON input for an array of object-union items", () => { + const objectBranch = (name: string): InspectorFormSchema => ({ + type: "object", + properties: { type: { type: "string", const: name } }, + required: ["type"], + }); + const schema: InspectorFormSchema = { + type: "object", + properties: { + items: { + title: "Items", + type: "array", + items: { anyOf: [objectBranch("A"), objectBranch("B")] }, + }, + }, + }; + renderWithMantine( + , + ); + expect(screen.getByLabelText(/Items/).tagName).toBe("TEXTAREA"); + }); + + it("falls back to the JSON input for a nullable array of object-union items", () => { + const objectBranch = (name: string): InspectorFormSchema => ({ + type: "object", + properties: { type: { type: "string", const: name } }, + required: ["type"], + }); + const schema: InspectorFormSchema = { + type: "object", + properties: { + items: { + title: "Items", + anyOf: [ + { type: "array", items: { anyOf: [objectBranch("A")] } }, + { type: "null" }, + ], + }, + }, + }; + renderWithMantine( + , + ); + expect(screen.getByLabelText(/Items/).tagName).toBe("TEXTAREA"); + }); + + // Mantine's select is string-valued, so a numeric const would submit "1" for + // 1 — the same wrong-type-on-the-wire problem that keeps a numeric enum off + // the select path. + it("falls back to the JSON input for non-string anyOf consts", () => { + const schema: InspectorFormSchema = { + type: "object", + properties: { + items: { + title: "Items", + type: "array", + items: { anyOf: [{ const: 1 }, { const: 2 }] }, + }, + }, + }; + renderWithMantine( + , + ); + expect(screen.getByLabelText(/Items/).tagName).toBe("TEXTAREA"); + }); + + it("falls back to the JSON input when two anyOf branches share a const", () => { + const schema: InspectorFormSchema = { + type: "object", + properties: { + items: { + title: "Items", + type: "array", + items: { anyOf: [{ const: "dup" }, { const: "dup" }] }, + }, + }, + }; + renderWithMantine( + , + ); + expect(screen.getByLabelText(/Items/).tagName).toBe("TEXTAREA"); + }); + + it("falls back to a text input for a string oneOf with no consts", () => { + const schema: InspectorFormSchema = { + type: "object", + properties: { + mode: { + title: "Mode", + type: "string", + oneOf: [{ type: "string" }, { type: "string" }], + }, + }, + }; + renderWithMantine( + , + ); + expect(screen.getByLabelText(/Mode/).tagName).toBe("INPUT"); + }); + + it("still renders a MultiSelect when every anyOf branch has a distinct const", () => { + const schema: InspectorFormSchema = { + type: "object", + properties: { + items: { + title: "Items", + type: "array", + items: { anyOf: [{ const: "a", title: "Alpha" }, { const: "b" }] }, + }, + }, + }; + renderWithMantine( + , + ); + expect(screen.getByRole("textbox", { name: "Items" })).toBeInTheDocument(); + }); + + // Reachable only since the collapse landed: the nullable wrapper had no + // top-level `type` before, so it fell to the JSON editor rather than into a + // string-valued MultiSelect that would submit ["1"] for [1]. + it("falls back to the JSON input for a nullable array of numeric item enums", () => { + // Wire data again — a numeric `enum` is valid JSON Schema but outside + // `InspectorFormSchema`'s `string[]`, so it comes in through the narrow. + const schema = toFormSchema({ + type: "object", + properties: { + levels: { + title: "Levels", + anyOf: [{ type: "array", items: { enum: [1, 2] } }, { type: "null" }], + }, + }, + }); + renderWithMantine( + , + ); + expect(screen.getByLabelText(/Levels/).tagName).toBe("TEXTAREA"); + }); + + // The sibling enum rules null out even though the type list names it, so the + // field must not get a clear button that emits a value the schema rejects. + it("offers no clear button when a sibling enum excludes null", () => { + const schema = toFormSchema({ + type: "object", + properties: { + direction: { + title: "Direction", + type: ["string", "null"], + enum: ["envio", "recebimento"], + }, + }, + }); + renderWithMantine( + , + ); + expect( + screen.queryByRole("button", { hidden: true }), + ).not.toBeInTheDocument(); + }); + + it("still falls back to the JSON input for a union of two real types", () => { + const schema: InspectorFormSchema = { + type: "object", + properties: { + mixed: { + title: "Mixed", + anyOf: [{ type: "string" }, { type: "number" }], + }, + }, + }; + renderWithMantine( + , + ); + expect(screen.getByLabelText(/Mixed/).tagName).toBe("TEXTAREA"); + }); +}); + +// #2020: a field holding text it cannot turn into a value reports `undefined`, +// which is exactly what an *empty* field reports. That makes the two states +// indistinguishable to the caller, so invalid text in an optional field was +// submittable and simply arrived at the server absent. The form reports draft +// validity directly so a submit gate can see what `values` cannot. +describe("SchemaForm draft validity (#2020)", () => { + const jsonSchema: InspectorFormSchema = { + type: "object", + properties: { + config: { type: "array", title: "Config" }, + }, + }; + + function ValidityHarness({ + schema, + onValidityChange, + resetKey, + }: { + schema: InspectorFormSchema; + onValidityChange: (hasInvalidDraft: boolean) => void; + resetKey?: string; + }) { + const [values, setValues] = useState>({}); + return ( + + ); + } + + it("reports an empty optional field as valid", () => { + const onValidityChange = vi.fn(); + renderWithMantine( + , + ); + expect(onValidityChange).toHaveBeenLastCalledWith(false); + }); + + it("reports an unparseable JSON draft, then clears it once the text parses", async () => { + const user = userEvent.setup(); + const onValidityChange = vi.fn(); + renderWithMantine( + , + ); + + const jsonInput = screen.getByLabelText(/Config/) as HTMLTextAreaElement; + await user.type(jsonInput, "[[1,"); + expect(onValidityChange).toHaveBeenLastCalledWith(true); + + await user.type(jsonInput, "2]"); + expect(onValidityChange).toHaveBeenLastCalledWith(false); + }); + + it("reports a number this client cannot send exactly, and says so on the field", async () => { + const user = userEvent.setup(); + const onValidityChange = vi.fn(); + const schema: InspectorFormSchema = { + type: "object", + properties: { + divisor: { type: "number", title: "Divisor" }, + }, + }; + renderWithMantine( + , + ); + + // Past MAX_SAFE_INTEGER the field reports no value rather than a rounded + // one, so — like the JSON editor — the text on screen would otherwise be + // submitted as an absent argument. + const input = screen.getByLabelText(/Divisor/) as HTMLInputElement; + await user.type(input, "90071992547409910"); + expect(onValidityChange).toHaveBeenLastCalledWith(true); + expect(screen.getByText(/this field will be omitted/)).toBeInTheDocument(); + + await user.clear(input); + expect(onValidityChange).toHaveBeenLastCalledWith(false); + expect( + screen.queryByText(/this field will be omitted/), + ).not.toBeInTheDocument(); + }); + + it("stays invalid while any one field is invalid", async () => { + const user = userEvent.setup(); + const onValidityChange = vi.fn(); + const schema: InspectorFormSchema = { + type: "object", + properties: { + first: { type: "array", title: "First" }, + second: { type: "array", title: "Second" }, + }, + }; + renderWithMantine( + , + ); + + await user.type(screen.getByLabelText(/First/), "x"); + await user.type(screen.getByLabelText(/Second/), "y"); + expect(onValidityChange).toHaveBeenLastCalledWith(true); + + // One of the two recovering is not enough. + await user.clear(screen.getByLabelText(/First/)); + expect(onValidityChange).toHaveBeenLastCalledWith(true); + + await user.clear(screen.getByLabelText(/Second/)); + expect(onValidityChange).toHaveBeenLastCalledWith(false); + }); + + it("surfaces a nested object's invalid draft through the outer form", async () => { + const user = userEvent.setup(); + const onValidityChange = vi.fn(); + const schema: InspectorFormSchema = { + type: "object", + properties: { + outer: { + type: "object", + title: "Outer", + properties: { + config: { type: "array", title: "Config" }, + }, + }, + }, + }; + renderWithMantine( + , + ); + + await user.type(screen.getByLabelText(/Config/), "x"); + expect(onValidityChange).toHaveBeenLastCalledWith(true); + }); + + it("clears a stale invalid draft when the form moves to another entity", async () => { + const user = userEvent.setup(); + const onValidityChange = vi.fn(); + const { rerender } = renderWithMantine( + , + ); + + await user.type(screen.getByLabelText(/Config/), "x"); + expect(onValidityChange).toHaveBeenLastCalledWith(true); + + // Switching entities remounts the field, discarding its draft — so text + // typed for the previous one must not keep the new one blocked. + rerender( + , + ); + expect(onValidityChange).toHaveBeenLastCalledWith(false); + }); + + it("reports valid when the form unmounts holding an invalid draft", async () => { + const user = userEvent.setup(); + const onValidityChange = vi.fn(); + const { unmount } = renderWithMantine( + , + ); + + await user.type(screen.getByLabelText(/Config/), "x"); + expect(onValidityChange).toHaveBeenLastCalledWith(true); + + unmount(); + expect(onValidityChange).toHaveBeenLastCalledWith(false); + }); + + it("renders without a validity callback", async () => { + const user = userEvent.setup(); + renderWithMantine( + , + ); + await user.type(screen.getByLabelText(/Config/), "x"); + expect(screen.getByText(/Not valid JSON/)).toBeInTheDocument(); + }); +}); diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx index 232b5327..54e50b6e 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx @@ -8,10 +8,17 @@ import { Text, TextInput, } from "@mantine/core"; -import { useState } from "react"; +import { useCallback, useEffect, useRef, useState } from "react"; import { ClearButton } from "../../elements/ClearButton/ClearButton"; import { useValueChange } from "../../../hooks/useValueChange"; -import type { InspectorFormSchema } from "../../../utils/jsonUtils"; +import type { + InspectorFormSchema, + JsonSchemaConst, +} from "../../../utils/jsonUtils"; +import { + isStringEnum, + normalizeNullableUnion, +} from "@inspector/core/json/nullableUnion.js"; const FieldLabel = Text.withProps({ fw: 500, @@ -51,6 +58,58 @@ function toEnumData( return values; } +/** + * Build `Select`/`MultiSelect` option data from a list of `oneOf`/`anyOf` + * branches that are expected to be constants. + * + * Returns `null` when the branches are not usable as options, which sends the + * field to the JSON fallback instead. Four ways that happens: + * + * - **A non-string `const`.** Mantine's select is string-valued, so + * `anyOf: [{ const: 1 }, { const: 2 }]` would submit `["1"]` where the + * schema says `[1]` — the same wrong-type-on-the-wire problem that keeps a + * numeric `enum` off the select path. An inspector must not misreport what + * it sends, so this stays on the JSON editor, where the value keeps its type. + * - **No `const` at all.** An `anyOf` of *object* schemas — what + * `z.array(z.union([z.object(…), z.object(…)]))` compiles to — has no + * top-level `const` on any branch, so every option would be the empty + * string. Mantine **throws** on duplicate option values, which greys out the + * whole tool panel rather than degrading (#2007). + * - **Duplicate values.** Two branches sharing a `const` throw the same way. + * - **An empty option value**, which Mantine cannot render as selectable. + * + * A union of object shapes has no faithful dropdown anyway, so the JSON editor + * is the honest widget for it rather than a lossy or crashing one. + */ +function toConstOptions( + branches: (InspectorFormSchema | JsonSchemaConst)[], +): { value: string; label: string }[] | null { + const options: { value: string; label: string }[] = []; + const seen = new Set(); + for (const branch of branches) { + if (typeof branch.const !== "string") { + return null; + } + const value = branch.const; + if (value === "" || seen.has(value)) { + return null; + } + seen.add(value); + options.push({ value, label: branch.title ?? value }); + } + return options.length > 0 ? options : null; +} + +/** + * Whether an enum `Select` should offer a clear affordance. Only a schema that + * admits `null` does: clearing emits `null`, and an enum without a null branch + * would reject it. Without this a nullable enum is a one-way door — once a + * value is picked there is no way back to "no answer". + */ +function isClearable(fieldSchema: InspectorFormSchema): boolean { + return fieldSchema.nullable === true; +} + /** * Interpret whatever Mantine's `NumberInput` reported as the JSON value for the * field. Anything that is not a finite number becomes `undefined`, which is how @@ -90,7 +149,162 @@ function toNumericValue(raw: string | number): number | undefined { return Number.isSafeInteger(Math.trunc(parsed)) ? parsed : undefined; } +/** Parse editor text, reporting `undefined` for anything that is not JSON yet. */ +function parseJsonDraft(text: string): unknown { + if (text.trim() === "") { + return undefined; + } + try { + return JSON.parse(text); + } catch { + return undefined; + } +} + +/** Structural equality for the draft/value re-sync, via canonical JSON. */ +function isSameJson(a: unknown, b: unknown): boolean { + if (a === undefined || b === undefined) { + return a === b; + } + return serializeJson(a) === serializeJson(b); +} + +/** + * How a field tells the enclosing `SchemaForm` whether the text it is holding + * can be sent. Keyed by field name rather than closed over per field so the + * reporter is referentially stable, which is what keeps the effect below from + * re-subscribing on every render. + */ +type DraftValidityReporter = (fieldName: string, isValid: boolean) => void; + +/** + * Publish a field's draft validity to the enclosing `SchemaForm`. + * + * Reported from an effect rather than from the change handler because validity + * also moves when the *parent* replaces the value — a cleared form or a loaded + * example re-syncs the draft and makes invalid text valid again without the + * user touching the field. Reporting during that render instead would mean + * updating another component mid-render, which React rejects. + * + * The cleanup reports the field as valid again, so a field that goes away — + * unmounted with the panel, or remounted under a new `resetKey` when the form + * moves to another entity — cannot leave a stale draft blocking submission. + */ +function useDraftValidity( + fieldName: string, + isValid: boolean, + onValidityChange: DraftValidityReporter, +): void { + useEffect(() => { + onValidityChange(fieldName, isValid); + return () => onValidityChange(fieldName, true); + }, [fieldName, isValid, onValidityChange]); +} + +interface SchemaJsonFieldProps { + fieldName: string; + label: string; + description?: string; + withAsterisk: boolean; + disabled: boolean; + value: unknown; + onChange: (value: unknown) => void; + onValidityChange: DraftValidityReporter; +} + +/** + * A `JsonInput` that keeps the text the user is typing, not just the value it + * currently parses to. + * + * This field used to drive the box straight off the parent's value and, when + * the text failed `JSON.parse`, store the **raw text** back as the value — which + * the next render re-`JSON.stringify`d, adding a layer of escaping per + * keystroke. Typing `[` gave `"["`, then `"\"[\""`, and the field was unusable + * within a few characters. That compounding escape is the original #1928 + * symptom, and it lived in this fallback rather than in the dispatch. + * + * Routing nullable fields to real widgets removed the common way of *landing* + * here, but it did not fix the editor — and #2007's fix deliberately sends a + * union of object shapes to it, so the editor itself has to be typeable. Hence + * the same split `SchemaNumberInput` uses: the raw text is the source of truth + * for what is *displayed*, while the parent only ever sees parsed JSON. + * + * While the text is mid-edit and does not parse, the parent is told + * `undefined` rather than handed the text. An inspector must not report a value + * it cannot send, and `undefined` is how "no value supplied" is spelled + * everywhere else in this form. + * + * That leaves invalid text and an empty field indistinguishable *to the value*, + * so the field reports the difference two other ways: it says so on itself via + * `error`, and it reports draft validity up through `onValidityChange` so the + * enclosing form's caller can disable Execute/Open App/Submit (#2020). Without + * the second channel an **optional** field's invalid text is simply dropped + * from a submission the user was allowed to make. + */ +function SchemaJsonField({ + fieldName, + value, + onChange, + onValidityChange, + ...inputProps +}: SchemaJsonFieldProps) { + const [draft, setDraft] = useState(() => + value === undefined ? "" : serializeJson(value), + ); + + // Re-sync only when the parent's value genuinely diverges from what the draft + // parses to, which leaves an external reset (a cleared form, a loaded + // example) working while an in-progress `[` — whose parse is `undefined`, + // matching the `undefined` we just emitted — is left alone. + useValueChange(value, (next) => { + if (!isSameJson(parseJsonDraft(draft), next)) { + setDraft(next === undefined ? "" : serializeJson(next)); + } + }); + + // 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); + + return ( + { + setDraft(text); + onChange(parseJsonDraft(text)); + }} + /> + ); +} + +/** + * Whether a number field's draft is text that cannot be sent. + * + * Anything non-empty that `toNumericValue` declines is invalid: a half-typed + * `"-"`, and — the case that matters — an integer past `MAX_SAFE_INTEGER`, + * which is deliberately dropped rather than silently rounded (see + * `toNumericValue`). Both would otherwise submit as an absent argument while + * the text sits in the box. + */ +function hasInvalidNumericDraft(draft: string | number): boolean { + if (typeof draft === "number") { + return false; + } + return draft.trim() !== "" && toNumericValue(draft) === undefined; +} + interface SchemaNumberInputProps { + fieldName: string; label: string; description?: string; withAsterisk: boolean; @@ -100,6 +314,7 @@ interface SchemaNumberInputProps { max?: number; allowDecimal: boolean; onChange: (value: number | undefined) => void; + onValidityChange: DraftValidityReporter; } /** @@ -126,8 +341,10 @@ interface SchemaNumberInputProps { * prop for the case it covers. */ function SchemaNumberInput({ + fieldName, value, onChange, + onValidityChange, ...inputProps }: SchemaNumberInputProps) { const [draft, setDraft] = useState(value ?? ""); @@ -138,10 +355,19 @@ function SchemaNumberInput({ } }); + // Same split as the JSON field: text this client cannot send reports no value, + // so it has to say so on the field and report it upward, or the argument is + // dropped from a submission the user was allowed to make (#2020). + const invalidDraft = hasInvalidNumericDraft(draft); + useDraftValidity(fieldName, !invalidDraft, onValidityChange); + return ( { setDraft(next); onChange(toNumericValue(next)); @@ -174,6 +400,21 @@ export interface SchemaFormProps { * substitute — callers rebuild it every render, so its identity is unstable. */ resetKey?: string; + /** + * Called with `true` while any field is holding text that cannot be sent — + * unparseable JSON, or a number this client cannot represent exactly. Both + * report their value as `undefined`, which is indistinguishable from an empty + * field once it reaches `values`, so a caller cannot derive this from the + * values it already has (#2020). + * + * Gate the submit action on it: `hasMissingRequiredFields` covers a field with + * *no* value, and this covers a field whose visible text produced none. An + * empty optional field is unaffected — it stays valid and submittable. + * + * The callback is read through a ref, so an inline closure is fine — it is + * called when the answer changes, not on every render. + */ + onValidityChange?: (hasInvalidDraft: boolean) => void; } function getDefaultValue(fieldSchema: InspectorFormSchema): unknown { @@ -199,15 +440,70 @@ export function SchemaForm({ onChange, disabled = false, resetKey, + onValidityChange, }: SchemaFormProps) { const properties = schema.properties ?? {}; const requiredFields = schema.required ?? []; + // The names of fields currently holding unsendable text. Held here rather + // than in each field because only the form sees them all, and only the form + // knows when the last one cleared. + const [invalidFields, setInvalidFields] = useState>( + () => new Set(), + ); + + // Stable so a field's reporting effect subscribes once, not per render. The + // updater returns the previous set unchanged when nothing moved, which is + // what makes a nested form's inline callback safe to call repeatedly. + const reportFieldValidity = useCallback( + (fieldName, isValid) => { + setInvalidFields((previous) => { + if (previous.has(fieldName) === !isValid) { + return previous; + } + const next = new Set(previous); + if (isValid) { + next.delete(fieldName); + } else { + next.add(fieldName); + } + return next; + }); + }, + [], + ); + + // Read through a ref so the callback's identity is not a dependency. It has + // to be one or the other, and a *stable* dependency is what this needs: a + // nested form is handed a fresh closure every render, and re-running the + // effect for that alone would toggle the parent's state (cleanup, then + // re-report), re-render this form, and loop forever. + const notifyValidity = useRef(onValidityChange); + useEffect(() => { + notifyValidity.current = onValidityChange; + }); + + // The cleanup mirrors the one in `useDraftValidity`: a form that is no longer + // rendered holds no drafts, so an unmounted nested form must not leave the + // outer one blocked. It runs in the same commit as the re-report, so the + // transient `false` is never rendered. + const hasInvalidDraft = invalidFields.size > 0; + useEffect(() => { + notifyValidity.current?.(hasInvalidDraft); + return () => notifyValidity.current?.(false); + }, [hasInvalidDraft]); + function handleFieldChange(fieldName: string, fieldValue: unknown) { onChange({ ...values, [fieldName]: fieldValue }); } - function renderField(fieldName: string, fieldSchema: InspectorFormSchema) { + function renderField(fieldName: string, rawSchema: InspectorFormSchema) { + // Flatten a nullable union (`anyOf: [X, {type:"null"}]`, `type: [X,"null"]`) + // before dispatching. Every branch below tests a single `type` string, so + // without this an "optional and explicitly nullable" field — what Zod's + // `.nullish()` emits — matches nothing and falls through to the raw-JSON + // fallback, which is unusable for a value the user has to type (#1928). + const fieldSchema = normalizeNullableUnion(rawSchema); const isRequired = requiredFields.includes(fieldName); const label = fieldSchema.title ?? fieldName; const description = fieldSchema.description; @@ -223,6 +519,7 @@ export function SchemaForm({ withAsterisk={isRequired} disabled={disabled} data={toEnumData(fieldSchema.enum, fieldSchema.enumNames)} + clearable={isClearable(fieldSchema)} value={(rawValue as string) ?? null} onChange={(val) => handleFieldChange(fieldName, val)} /> @@ -230,11 +527,11 @@ export function SchemaForm({ } // string with oneOf - if (fieldSchema.type === "string" && fieldSchema.oneOf) { - const data = fieldSchema.oneOf.map((item) => ({ - value: String(item.const ?? ""), - label: item.title ?? String(item.const ?? ""), - })); + const oneOfData = fieldSchema.oneOf + ? toConstOptions(fieldSchema.oneOf) + : null; + if (fieldSchema.type === "string" && oneOfData) { + const data = oneOfData; return (