From df399272f2ad1452dfa0f713e7b7408bced93096 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue, 11 Aug 2026 15:10:17 +0000 Subject: [PATCH 01/25] build(deps): bump the github-actions group across 1 directory with 5 updates Bumps the github-actions group with 5 updates in the / directory: | Package | From | To | | --- | --- | --- | | [actions/checkout](https://github.com/actions/checkout) | `6` | `7` | | [actions/setup-node](https://github.com/actions/setup-node) | `6` | `7` | | [actions/cache](https://github.com/actions/cache) | `4` | `6` | | [docker/setup-qemu-action](https://github.com/docker/setup-qemu-action) | `3` | `4` | | [docker/build-push-action](https://github.com/docker/build-push-action) | `6` | `7` | Updates `actions/checkout` from 6 to 7 - [Release notes](https://github.com/actions/checkout/releases) - [Commits](https://github.com/actions/checkout/compare/v6...v7) Updates `actions/setup-node` from 6 to 7 - [Release notes](https://github.com/actions/setup-node/releases) - [Commits](https://github.com/actions/setup-node/compare/v6...v7) Updates `actions/cache` from 4 to 6 - [Release notes](https://github.com/actions/cache/releases) - [Changelog](https://github.com/actions/cache/blob/main/RELEASES.md) - [Commits](https://github.com/actions/cache/compare/v4...v6) Updates `docker/setup-qemu-action` from 3 to 4 - [Release notes](https://github.com/docker/setup-qemu-action/releases) - [Commits](https://github.com/docker/setup-qemu-action/compare/v3...v4) Updates `docker/build-push-action` from 6 to 7 - [Release notes](https://github.com/docker/build-push-action/releases) - [Commits](https://github.com/docker/build-push-action/compare/v6...v7) --- updated-dependencies: - dependency-name: actions/checkout dependency-version: '7' dependency-type: direct:production update-type: version-update:semver-major dependency-group: github-actions - dependency-name: actions/setup-node dependency-version: '7' dependency-type: direct:production update-type: version-update:semver-major dependency-group: github-actions - dependency-name: actions/cache dependency-version: '6' dependency-type: direct:production update-type: version-update:semver-major dependency-group: github-actions - dependency-name: docker/setup-qemu-action dependency-version: '4' dependency-type: direct:production update-type: version-update:semver-major dependency-group: github-actions - dependency-name: docker/build-push-action dependency-version: '7' dependency-type: direct:production update-type: version-update:semver-major dependency-group: github-actions ... Signed-off-by: dependabot[bot] --- .github/workflows/main.yml | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) 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 From bb599e5057d89e96a46b0d22faf454e821bda9bf Mon Sep 17 00:00:00 2001 From: cliffhall Date: Tue, 11 Aug 2026 15:28:30 -0400 Subject: [PATCH 02/25] =?UTF-8?q?docs:=20add=20the=20six-month=20Inspector?= =?UTF-8?q?=20roadmap=20(Aug=202026=20=E2=86=92=20Feb=202027)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Through v1 the Inspector was a follow-along project — the spec moved, we chased it, and whatever planning capacity was left went to keeping up rather than to the tool's own design. v2 meeting the 2026-07-28 spec across all three clients lifts that constraint, but nothing written down said so, and there was no shared list of what the MCP roadmap is likely to demand of us next. Add docs/inspector-roadmap-2026-h2.md covering 2026-08-11 → 2027-02-11 (~26 weekly milestones, v2.2.0 → ~v2.27.0), in two explicitly-budgeted tracks. Track A predicts Inspector features per MCP roadmap theme and WG charter, each tagged build-now / design-now-build-on-signal / watch, so a speculative item gets a liaison rather than code: transports and session lifecycle, Server Cards (SEP-2127), Tasks retry/expiry and the extension→core migration, enterprise audit + ID-JAG, triggers/events, result types, interceptors (SEP-1763), file inputs (SEP-2356), skills (SEP-2640), primitive grouping, and conformance. Track B is the work we choose, headlined by the zoomable protocol/network timeline — lanes, spans rather than points, brush-to-filter, MRTR and task grouping — then session record/replay, a shared diff primitive, a command palette, saved-call collections feeding CI assertions, the argument-editor workstream, and Connection Doctor. The sequencing argument is that several Track B items are force multipliers for Track A: each protocol feature arrives with a rendering problem, and one general timeline plus one general diff is cheaper than a bespoke panel per SEP. So the general surfaces come first and the SEP work renders into them. Two consolidations the survey surfaced, both worth acting on regardless of whether this plan is adopted as written: - #1853, #1856, #1885, #1928, #1919 and #1910 are one defect class, not six bugs — the argument editor is not schema-aware. Treating them separately has already produced one regression from v1.x (#1928). - #962, #1936, #1951, #1944 and #1914 are one missing feature: an ordered diagnostic that reports which connection step failed and what to do. Sourcing is the published roadmap plus the nine WG and six IG charters, cited in full at the end; the doc says so, and flags that Track A should be revised if internal planning carries themes the public page omits. Status is "Draft for WG review" — merging this starts the discussion rather than settling it. Adds one file and touches nothing else. The README guide list and the AGENTS.md docs/ tree entry are deliberately left for a follow-up rather than bundled here, to keep this diff to the document itself. Markdown-only, and outside every format and typecheck glob (SOURCE_EXTENSIONS has no "md"), so no gate is affected. Closes #1980 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013CaLPZQzfWDHoPxyL1vbYL Signed-off-by: cliffhall --- docs/inspector-roadmap-2026-h2.md | 570 ++++++++++++++++++++++++++++++ 1 file changed, 570 insertions(+) create mode 100644 docs/inspector-roadmap-2026-h2.md diff --git a/docs/inspector-roadmap-2026-h2.md b/docs/inspector-roadmap-2026-h2.md new file mode 100644 index 00000000..bfcee53d --- /dev/null +++ b/docs/inspector-roadmap-2026-h2.md @@ -0,0 +1,570 @@ +# Inspector Roadmap — August 2026 → February 2027 + +> A six-month plan for the Inspector client family (Web, CLI, TUI), covering both +> **spec-following work** driven by the MCP roadmap and **experience work** we choose +> for ourselves. + +**Horizon:** 2026-08-11 → 2027-02-11 (~26 weekly milestones, `v2.2.0` → ~`v2.27.0`) +**Owner:** [Inspector V2 WG](https://modelcontextprotocol.io/community/working-groups/inspector-v2) +**Status:** Draft for WG review + +--- + +## Table of Contents + +- [1. Why this document exists](#1-why-this-document-exists) +- [2. The two tracks](#2-the-two-tracks) +- [3. Track A — following the spec](#3-track-a--following-the-spec) + - [3.1 Transport evolution and scalability](#31-transport-evolution-and-scalability) + - [3.2 Server Cards](#32-server-cards) + - [3.3 Agent communication and Tasks](#33-agent-communication-and-tasks) + - [3.4 Enterprise readiness](#34-enterprise-readiness) + - [3.5 Triggers and events](#35-triggers-and-events) + - [3.6 Result type improvements](#36-result-type-improvements) + - [3.7 Interceptors](#37-interceptors) + - [3.8 File uploads](#38-file-uploads) + - [3.9 Skills over MCP](#39-skills-over-mcp) + - [3.10 Primitive grouping and tool annotations](#310-primitive-grouping-and-tool-annotations) + - [3.11 Conformance and validation](#311-conformance-and-validation) +- [4. Track B — experience work we choose](#4-track-b--experience-work-we-choose) + - [4.1 The zoomable timeline (headline)](#41-the-zoomable-timeline-headline) + - [4.2 Session record, replay, and share](#42-session-record-replay-and-share) + - [4.3 Diff and compare](#43-diff-and-compare) + - [4.4 Command palette and global search](#44-command-palette-and-global-search) + - [4.5 Saved calls and collections](#45-saved-calls-and-collections) + - [4.6 Assertions and CI flows](#46-assertions-and-ci-flows) + - [4.7 The argument editor workstream](#47-the-argument-editor-workstream) + - [4.8 Connection Doctor](#48-connection-doctor) + - [4.9 Server management and portability](#49-server-management-and-portability) + - [4.10 Workspace and layout](#410-workspace-and-layout) + - [4.11 Performance at scale](#411-performance-at-scale) + - [4.12 Accessibility and keyboard-first operation](#412-accessibility-and-keyboard-first-operation) + - [4.13 Onboarding](#413-onboarding) + - [4.14 Plugin architecture](#414-plugin-architecture) +- [5. Sequencing](#5-sequencing) +- [6. What we are deliberately not doing](#6-what-we-are-deliberately-not-doing) +- [7. Open questions](#7-open-questions) +- [8. Sources](#8-sources) + +--- + +## 1. Why this document exists + +Through v1, the Inspector was a **follow-along project**. The spec moved, we chased it, and +whatever planning capacity remained went to keeping up rather than to the tool's own design. +Every release was reactive by necessity. + +That constraint has lifted. v2 meets the 2026-07-28 spec across all three clients, on SDK v2, +with a shared `core/`, a ≥90% per-file coverage gate, and a smoke/e2e apparatus that catches +packaging failures. For the first time we can spend planned effort on **what the Inspector +should be**, not only on what the spec just became. + +This document splits the next six months into those two kinds of work, so that neither +starves the other. The explicit intent is a **roughly even split of capacity** — spec-following +work is non-negotiable but bounded, and the remaining capacity is ours to direct. + +> **Sourcing note.** The MCP roadmap circulated as a Google Doc ("MCP Roadmap Process and +> Timeline") requires authentication and could not be read directly. This plan is built from +> the **published** roadmap at `modelcontextprotocol.io/development/roadmap` (last updated +> 2026-03-05) plus the current WG and IG charters, which together cover the same themes at +> more implementation-relevant detail. If the private doc contains timelines or themes absent +> from the public page, §3 should be revised against it before the plan is adopted. + +--- + +## 2. The two tracks + +| | **Track A — Spec-following** | **Track B — Experience** | +| --------------------------- | ----------------------------------------------------- | ------------------------------------------- | +| **Driver** | MCP roadmap, WG deliverables, SEP acceptance | Our own judgment about the tool | +| **Trigger to start** | A SEP reaches Draft with a Tier-1 SDK reference impl | Whenever we have capacity | +| **Risk** | Slips when upstream slips; we cannot control the date | We control the date entirely | +| **Failure mode if starved** | Inspector stops being the reference test client | Inspector stays a protocol dump, not a tool | +| **Target capacity** | ~50% | ~50% | + +The two tracks are not independent. Several Track B items — the timeline, session +record/replay, diff — are **force multipliers for Track A**: each new protocol feature +arrives with a rendering problem, and a general timeline plus a general diff is cheaper than +one bespoke panel per SEP. That is the core scheduling argument of this plan: **build the +general surfaces early so the spec work that lands later is cheap to display.** + +### How the Inspector's role is changing + +Worth stating plainly, because it shapes the priorities below. The roadmap's Validation +section names **conformance test suites**, **SDK tiers**, and **reference implementations** as +standing investments, and SEP-2484 now requires conformance tests for final SEPs. The +Inspector is the most visible MCP client in the ecosystem and is already the thing people +reach for when a server misbehaves. + +That points at an expanded role: not just _"show me the traffic"_ but _"tell me whether this +server is correct."_ Several items below (Server Card diffing, the conformance runner, +assertions, the readiness summary) are steps toward that, and they should be evaluated as a +group rather than individually. + +--- + +## 3. Track A — following the spec + +Each subsection states the upstream theme, our read on what it means for the Inspector, and a +concrete feature list. **Confidence** flags how much of the list we can commit to now: + +- 🟢 **Build now** — the shape is known; blocked only on our own capacity. +- 🟡 **Design now, build on signal** — enough detail to design against; wait for a Draft SEP or a Tier-1 SDK impl before building. +- 🔴 **Watch** — too early to predict a UI; keep a tracking issue and a WG liaison. + +### 3.1 Transport evolution and scalability + +**Upstream:** Transports WG. Next-generation Streamable HTTP that runs statelessly across +multiple instances and behaves correctly behind load balancers and proxies; a session model +covering creation, resumption, and migration; conformance guidance for SDK authors. The +roadmap is explicit that **no additional official transports** ship this cycle. + +**Read:** This is the theme most likely to produce breaking wire changes, and the one where +the Inspector is most useful — session resumption and proxy behavior are exactly the failures +nobody can reproduce by reading code. Our era model (`legacy` / `modern` / `auto`) already +gives us the negotiation seam to add a third era behind. + +| Feature | Confidence | Notes | +| ------------------------------------------------------------------------------------------------------------------------------------------------------------ | ---------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| **Session lifecycle lane** — session id, creation, resumption, migration, and expiry as first-class events, not log lines | 🟢 | Renders into the timeline (§4.1). Buildable against today's session model; extends to the new one. | +| **`Last-Event-ID` resumption support and display** | 🟢 | Existing gap — [#920](https://github.com/modelcontextprotocol/inspector/issues/920). Do it now; it is table stakes for the new session work. | +| **Proxy / intermediary harness** — route through a configurable proxy, then deliberately misbehave: rewrite headers, drop the GET stream, close mid-response | 🟡 | Builds on [#1684](https://github.com/modelcontextprotocol/inspector/issues/1684). Needs a `misbehaving-proxy` preset in `test-servers/`. | +| **Stateless-mode verification** — issue the same request across N synthetic instances and diff the responses | 🟡 | Directly tests the property the WG is specifying. Pairs with §4.3. | +| **Third protocol era behind the existing negotiation seam** | 🟡 | Cost is low _if_ we keep era-conditional exposure rather than replacing the legacy path. | +| **Custom transport support** | 🟢 | [#1741](https://github.com/modelcontextprotocol/inspector/issues/1741). The roadmap pushes experimentation to custom transports, so the Inspector should be able to load one. | + +### 3.2 Server Cards + +**Upstream:** Server Card WG, [SEP-2127](https://github.com/modelcontextprotocol/modelcontextprotocol/pull/2127) (Draft). A standard `.well-known` document exposing structured server metadata so browsers, crawlers, and registries can discover capabilities **without connecting**. Deliberately kept close to a subset of `server.json`. + +**Read:** This is the single highest-leverage Track A item for us, because it creates a new +Inspector capability rather than a new panel: **inspect before connect**. It also creates an +obvious correctness question that only a tool like ours can answer. + +| Feature | Confidence | Notes | +| ----------------------------------------------------------------------------------------------------------- | ---------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| **Card preview** — paste a URL, fetch the card, render the capability surface, one-click add to catalog | 🟡 | The pre-connection entry point. Wait for the format to settle. | +| **Card-vs-reality diff** — compare the advertised card against what `initialize` + `*/list` actually return | 🟡 | _The_ Inspector-shaped feature here. Nobody else in the ecosystem is positioned to check this. Shares machinery with [#1034](https://github.com/modelcontextprotocol/inspector/issues/1034) and §4.3. | +| **`mcp-inspector --card-lint `** — validate a card, non-zero exit on drift | 🟡 | CI-usable; a natural companion to the conformance runner (§3.11). | +| **`server.json` support** | 🟢 | [#922](https://github.com/modelcontextprotocol/inspector/issues/922). Prerequisite — the card is a subset, so this lands first regardless. | + +### 3.3 Agent communication and Tasks + +**Upstream:** Agents WG. Tasks (`io.modelcontextprotocol/tasks`, SEP-2663) is being +**stabilized and promoted from an extension into core**. Named open gaps: **retry semantics** +(what happens on transient failure, who decides to retry) and **expiry policies** (result +retention, how clients learn a result expired). An Agents Extension is under evaluation. + +**Read:** We already drive the modern Tasks extension ourselves over a raw-wire channel, +because SDK v2 era-gates `tasks/*` out. Promotion to core will move that back under the SDK — +plan for the migration, but **keep the era-conditional exposure**; the legacy `capabilities.tasks` +path must keep working. + +| Feature | Confidence | Notes | +| -------------------------------------------------------------------------------------------------------------------- | ---------- | ------------------------------------------------------------------------------------------- | +| **Retry visualization** — attempts, backoff, who initiated each retry | 🟡 | Design against the WG's gap list now. | +| **Expiry / TTL surfacing** — retention countdown on a completed task, distinct rendering for an expired-result error | 🟡 | Cheap once the semantics land; easy to get wrong if we guess early. | +| **Tasks as timeline spans** — a long-running task is a span, not a row | 🟢 | Falls out of §4.1 for free. The strongest argument for building the timeline first. | +| **Extension → core migration** | 🟡 | Retire the raw-wire channel when the SDK covers it; keep both paths during overlap. | +| **`Mcp-Name` header on Tasks over Streamable HTTP** | 🟢 | [#1917](https://github.com/modelcontextprotocol/inspector/issues/1917) — open bug, fix now. | +| **Discover checkmarks for task extensions** | 🟢 | [#1887](https://github.com/modelcontextprotocol/inspector/issues/1887). | + +### 3.4 Enterprise readiness + +**Upstream:** An Enterprise WG is expected to form. Four named areas: **audit trails and +observability**, **enterprise-managed auth** (Cross-App Access / ID-JAG), **gateway and proxy +patterns**, and **configuration portability**. Most output is expected as extensions rather +than core spec changes. Related: the Enterprise-Managed Authorization IG, and sponsored work +on [SEP-1932 (DPoP)](https://github.com/modelcontextprotocol/modelcontextprotocol/pull/1932) and [SEP-1933 (Workload Identity Federation)](https://github.com/modelcontextprotocol/modelcontextprotocol/pull/1933). + +**Read:** "Audit trails and observability, in a form enterprises can feed into their existing +pipelines" is a description of something the Inspector nearly already has. We hold the entire +session; we simply cannot **export** it in any pipeline-shaped format. That gap is cheap to +close and disproportionately valuable. + +| Feature | Confidence | Notes | +| ------------------------------------------------------------------------------------------------------ | ---------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| **OTLP export** — emit the session as OpenTelemetry spans; show trace/span ids inline; "copy as trace" | 🟢 | SEP-414 already puts trace context in `_meta`. Buildable today, no upstream dependency. | +| **Structured audit transcript** — the full session as a stable, documented JSON artifact | 🟢 | Shares its format with §4.2 record/replay. Build once, use for both. | +| **Machine-readable readiness summary** | 🟢 | [#1916](https://github.com/modelcontextprotocol/inspector/issues/1916). | +| **ID-JAG / Cross-App Access test flow** | 🟡 | The EMA IG exists specifically because this only works when IdP + client + AS interoperate. A test client is exactly what they lack. Related: [#1937](https://github.com/modelcontextprotocol/inspector/issues/1937), [#571](https://github.com/modelcontextprotocol/inspector/issues/571). | +| **DPoP and Workload Identity Federation** | 🔴 | Both sponsored but pre-acceptance. Watch; do not build. | +| **Gateway mode** — declare an intermediary, then show what we sent vs. what the gateway forwarded | 🟡 | Depends on the Gateways IG settling propagation semantics. | +| **Configuration portability** | 🟢 | [#1912](https://github.com/modelcontextprotocol/inspector/issues/1912), [#904](https://github.com/modelcontextprotocol/inspector/issues/904), plus `server.json` (§3.2). | + +### 3.5 Triggers and events + +**Upstream:** Triggers and Events WG. A standardized server→client callback mechanism +(webhooks or similar), with subscription lifecycle and **ordering guarantees that hold across +all transports**. Status: "SEP: Events in MCP v1 RFC" — **Ideating**. + +**Read:** ⚠️ **This is the largest architectural change on the horizon for us, and the one we +are least prepared for.** Every Inspector surface today assumes we are the party that +_initiated_ the connection. A webhook mechanism makes us a **server** — we must host a +publicly reachable callback endpoint, which for a tool that usually runs on `localhost` is a +real problem (tunnels, port forwarding, or a relay). + +We should start the design conversation **now**, well ahead of the SEP, and bring it to the +WG as implementation feedback. The ordering-guarantee requirement in particular is +untestable without a client that records arrival order — which is us. + +| Feature | Confidence | Notes | +| ----------------------------------------------------------------------------------------- | ---------- | ------------------------------------------------------------------------------------------------------------------------------------------------ | +| **Callback receiver** — backend-hosted endpoint, its URL registered as the trigger target | 🔴 | Needs design now, build later. Security review mandatory: an inbound public endpoint on a process that spawns subprocesses is a serious surface. | +| **Local reachability story** — tunnel integration or documented guidance | 🔴 | Likely the hardest UX problem of the whole six months. | +| **Delivery log with ordering and duplicate assertions** | 🔴 | The conformance value: did events arrive in the promised order? were any redelivered? | + +### 3.6 Result type improvements + +**Upstream:** "On the Horizon." **Streamed results** (incremental output for generated text, +audio, video frames) and **reference-based results** (client decides when to pull a large +payload into context). Explicitly cross-cutting — streaming touches transport, references +touch the schema. + +**Read:** Streaming changes how every result panel renders: today we display a _result_, and +we would need to display a _stream that becomes a result_. Worth a rendering abstraction +before the SEP, not after. + +| Feature | Confidence | Notes | +| ------------------------------------------------------------------------------------------------------- | ---------- | ----------------------------------------------------------------------------------- | +| **Incremental result rendering** — progressive display, with time-to-first-chunk and inter-chunk timing | 🔴 | The timing view is Inspector-shaped; the timeline is the natural home. | +| **Reference-result handling** — show a handle plus an explicit "pull payload", with size accounting | 🔴 | Also a good default for large payloads _today_, independent of the SEP (see §4.11). | + +### 3.7 Interceptors + +**Upstream:** Interceptors WG, [SEP-1763](https://github.com/modelcontextprotocol/modelcontextprotocol/pull/2076) (Draft). Interceptors as a new primitive with two types — **validators** (pass/fail) and **mutators** (transform payloads) — across in-process, sidecar, and remote deployment models, with priority-based chain ordering and audit-mode semantics. A **CLI client for interceptor invocation and testing** is a listed WG deliverable (Ideating, unowned). + +**Read:** Two things stand out. First, "CLI client for interceptor invocation and testing" is +**an unclaimed deliverable that describes our CLI**. Worth raising with the WG — Ola co-leads +both groups, so the liaison already exists. Second, an interceptor chain is a +_before → after payload transformation_, which is a diff, which we should already be able to +render (§4.3). + +| Feature | Confidence | Notes | +| ---------------------------------------------------------------------------------------------------------- | ---------- | ---------------------------------------------------------------------------------------------------------------------- | +| **Interceptor test bench** — register a chain, show before/after diff per hop, visualize priority ordering | 🟡 | The clearest "Inspector as the reference tool" opportunity of the six months. | +| **Audit-mode rendering** — what _would_ have been blocked or mutated | 🟡 | Follows the SEP's audit semantics. | +| **CLI interceptor invocation** | 🟡 | **Action: raise with the Interceptors WG.** If we take it, it needs its own milestone allocation. | +| **Our plugin architecture as an interceptor host** | 🟡 | [#1025](https://github.com/modelcontextprotocol/inspector/issues/1025). Prevents us building two extension mechanisms. | + +### 3.8 File uploads + +**Upstream:** File Uploads WG, [SEP-2356](https://github.com/modelcontextprotocol/modelcontextprotocol/pull/2356) (Draft, TS SDK reference impl targeted End May). Declarative `FileInputDescriptor` on tool input schemas and elicitation schemas, so hosts render native file pickers. Success criteria explicitly include **"at least one production host rendering a native file picker from the descriptor."** + +**Read:** The most tractable Track A item on the list — narrow, well-specified, with a TS SDK +reference implementation coming, and we are a credible candidate for that "production host." +It touches three surfaces: `SchemaForm` (Tools), elicitation forms, and MCP Apps. + +| Feature | Confidence | Notes | +| ------------------------------------------------------------------------------------ | ---------- | ------------------------------------------------ | +| **File picker in `SchemaForm`** when a descriptor is present, with data-URI encoding | 🟡 | Wait for the TS SDK types, then build. Low risk. | +| **Same in elicitation forms** | 🟡 | Shared component. | +| **Size guardrails and host-side validation** | 🟡 | The SEP references OWASP ASVS V5. | + +### 3.9 Skills over MCP + +**Upstream:** Skills Over MCP WG, [SEP-2640](https://github.com/modelcontextprotocol/modelcontextprotocol/pull/2640) (In Review, Extensions Track). Resources-based; a reference implementation is also In Review. + +**Read:** Because it is Resources-based, the incremental cost is low — a Skills view over the +existing resource machinery rather than a new subsystem. + +| Feature | Confidence | Notes | +| -------------------------------------------------------- | ---------- | -------------------------------------------------------------------- | +| **Skills view** — list, preview content, show activation | 🟡 | Gate on the negotiated extension, the way the Tasks tab gates today. | + +### 3.10 Primitive grouping and tool annotations + +**Upstream:** Two IGs. **Primitive Grouping** explores organizing Tools/Resources/Prompts +beyond flat lists — deliberately not picking one canonical pattern early. **Tool Annotations** +is consolidating six independent annotation SEPs and considering runtime annotations and tool +_response_ annotations. + +**Read:** Grouping is the rare case where the spec-following work and the UX work are the same +work. Flat lists are already our weakest surface on large servers — [#1957](https://github.com/modelcontextprotocol/inspector/issues/1957) (duplicate tool names) was a symptom. **Build the grouped sidebar as a UX +improvement now**, and adopt whatever grouping the IG lands as a data source later. + +| Feature | Confidence | Notes | +| ------------------------------------------------------------------ | ---------- | --------------------------------------------------------------------------------------------------------------------- | +| **Grouped / tree sidebars with group-aware search** | 🟢 | Build now on client-side heuristics (name prefixes, annotations). Ship value immediately; swap the data source later. | +| **Richer annotation rendering** | 🟢 | Extends the existing `AnnotationBadge`. | +| **Annotation-driven confirmation** before a `destructiveHint` call | 🟢 | Small, obviously correct, no upstream dependency. | +| **Runtime / response annotations** | 🔴 | Watch. | + +### 3.11 Conformance and validation + +**Upstream:** Standing investment — conformance test suites, SDK tiers ([SEP-1730](https://github.com/modelcontextprotocol/modelcontextprotocol/issues/1730)), reference implementations. [SEP-2484](https://github.com/modelcontextprotocol/modelcontextprotocol/pull/2484) now **requires conformance tests for final SEPs**, and the EMA IG is explicitly contributing scenarios to the `modelcontextprotocol/conformance` repository. + +**Read:** A conformance suite needs a driver and a report. We are the natural driver, and we +already have a CLI that exits non-zero. This is the clearest path to the expanded role +described in §2 — and unlike most of Track A, **it is not gated on any SEP**. + +| Feature | Confidence | Notes | +| ------------------------------------------------------------------------------------------------- | ---------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| **Conformance runner** — run the suite against a connected server, render pass/fail per assertion | 🟡 | Needs coordination on the suite's programmatic interface. **Action: open a conversation with the conformance maintainers.** | +| **`mcp-inspector --conformance` for CI** | 🟡 | Same engine, CLI report, exit code. | +| **Strict schema validation with actionable errors** | 🟢 | [#1005](https://github.com/modelcontextprotocol/inspector/issues/1005), [#1015](https://github.com/modelcontextprotocol/inspector/issues/1015). No dependency; start here. | + +--- + +## 4. Track B — experience work we choose + +Nothing in this section waits on a SEP. Ordered by leverage, not by effort. + +### 4.1 The zoomable timeline (headline) + +**Committed.** The single feature that most changes what the Inspector _is_. + +The Protocol and Network screens are chronological lists. A list answers "what happened next" +but not "what happened _at the same time_", "how long did this take", or "which of these +caused that" — and those are the questions people actually bring to the Inspector. A +session with an MRTR round-trip, a long-running task, a subscription stream, and a +mid-session OAuth step-up is, in list form, an interleaved mess. On a time axis it is legible +at a glance. + +**Design sketch:** + +- **A third view over the existing stores**, not a new data path. Protocol, Network, and + Timeline become three renderings of one session. This keeps the coverage gate and the + existing `protocolUtils` derivations intact. +- **Lanes**, each independently collapsible: + `client → server` · `server → client` · notifications · tasks · subscription streams · OAuth/auth · errors +- **Spans, not points.** A request occupies from send to response; a task occupies its whole + lifetime; a stream is a bar with events on it. Duration becomes visible, which is most of + the value. +- **Zoom and pan** across the full range, from whole-session down to sub-millisecond. + Brush-to-select a range and filter every other view to it. +- **Grouping** — an MRTR conversation is one collapsible span containing its rounds; a task + contains its polls. +- **Click through** to the existing Protocol/Network entry. The timeline is navigation, not a + replacement. +- **A pinned mini-timeline strip** above every tab, so a spike is visible while you are in + Tools, and clicking it jumps to the full view. +- **Latency distribution** as a secondary view — per method, so a slow tool is obvious. +- **Virtualized**, keyboard-navigable, and rendered from the same store the other views use. + +**Deliberately out of scope for v1 of this feature:** cross-server correlation (needs §4.10), +and OTLP-shaped nesting (needs §3.4). + +### 4.2 Session record, replay, and share + +Save a complete session — protocol log, network log, server config, negotiated capabilities — +to a single file. Reopen it later, on another machine, with no server running. Attach it to a +bug report. + +This changes issue triage from "works on my machine" into an artifact, and it is the same +serialization format as the enterprise audit transcript (§3.4) — **build the format once**. +Replay also gives us fixtures: a recorded session is a regression test. + +### 4.3 Diff and compare + +Two sessions, or two servers, side by side. Concretely: + +- **Capability diff** — reconnect after changing your server, see exactly what moved in + `tools/list` / `resources/list` / `prompts/list`. ([#1034](https://github.com/modelcontextprotocol/inspector/issues/1034)) +- **Session diff** — same calls, two servers, what differed. +- **Payload diff** — before/after for any pair of JSON documents. + +The payload differ is a **shared primitive**: interceptor before/after (§3.7), Server +Card-vs-reality (§3.2), and stateless-instance comparison (§3.1) are all the same widget with +different inputs. Build it as a component first, then wire the three consumers. + +### 4.4 Command palette and global search + +`⌘K` to jump to any server, tool, resource, or prompt; re-run the last call; switch tabs. Plus +full-text search across the protocol log with a real filter syntax (`method:tools/call +status:error duration:>500ms`). The Inspector is currently a mouse-driven app; for a developer +tool that is a daily tax. + +### 4.5 Saved calls and collections + +Name a tool call with its arguments, save it, re-run it, parameterize it, share it. A +Postman-collection model for MCP. The single most requested shape of workflow improvement for +any protocol client, and it composes directly with §4.6. + +### 4.6 Assertions and CI flows + +Attach expectations to a saved call — result matches schema, field equals value, latency under +a bound — and run the collection from the CLI with a non-zero exit on failure. This turns the +Inspector from an interactive tool into part of a server author's test suite, and it shares an +engine with the conformance runner (§3.11). +Related: [#1005](https://github.com/modelcontextprotocol/inspector/issues/1005), [#1886](https://github.com/modelcontextprotocol/inspector/issues/1886), [#1916](https://github.com/modelcontextprotocol/inspector/issues/1916). + +### 4.7 The argument editor workstream + +Six open issues are all the same defect class — the argument editor is not schema-aware: + +| Issue | Symptom | +| ---------------------------------------------------------------------- | ------------------------------------------------------------------- | +| [#1853](https://github.com/modelcontextprotocol/inspector/issues/1853) | JSON parameter editor escaping while typing | +| [#1856](https://github.com/modelcontextprotocol/inspector/issues/1856) | Backspace recursively escapes JSON tool inputs | +| [#1885](https://github.com/modelcontextprotocol/inspector/issues/1885) | Null values corrupted with cascading escapes | +| [#1928](https://github.com/modelcontextprotocol/inspector/issues/1928) | Nullable enums fall back to a broken raw Textarea (v1.x regression) | +| [#1919](https://github.com/modelcontextprotocol/inspector/issues/1919) | Resource templates lack RFC 6570 expansion | +| [#1910](https://github.com/modelcontextprotocol/inspector/issues/1910) | Complex `_meta` not expressible | + +**Fix them as one workstream, not six bugs.** A proper schema-aware editor (CodeMirror or +Monaco with JSON Schema integration) resolves the class and unblocks file inputs (§3.8) and +strict validation (§3.11). Treating them individually has already produced one regression from +v1. + +### 4.8 Connection Doctor + +Connection failures are currently opaque, and five open issues say so +([#962](https://github.com/modelcontextprotocol/inspector/issues/962), [#1936](https://github.com/modelcontextprotocol/inspector/issues/1936), [#1951](https://github.com/modelcontextprotocol/inspector/issues/1951), [#1944](https://github.com/modelcontextprotocol/inspector/issues/1944), [#1914](https://github.com/modelcontextprotocol/inspector/issues/1914)). + +Run an ordered checklist on failure — DNS · TCP · TLS (including local-cert cases) · +`/.well-known` discovery · protocol version negotiation · auth — and report **which step +failed and what to do about it**. First-connection success is the entire first impression of +the tool, and today a `https://localhost` server or a dev container silently fails. + +Bundle the related fixes: `*.localhost` domains ([#1944](https://github.com/modelcontextprotocol/inspector/issues/1944)), the trusted-local-host OAuth HTTP +exception ([#1911](https://github.com/modelcontextprotocol/inspector/issues/1911)), and the ghost-server entry left by a failed manual connect ([#1914](https://github.com/modelcontextprotocol/inspector/issues/1914)). + +### 4.9 Server management and portability + +Already well represented on the board; grouping it here so it is scheduled as a theme rather +than piecemeal: rich server configuration ([#1857](https://github.com/modelcontextprotocol/inspector/issues/1857)), custom headers and cookies ([#1915](https://github.com/modelcontextprotocol/inspector/issues/1915)), +auth/token URL overrides ([#1906](https://github.com/modelcontextprotocol/inspector/issues/1906)), file-backed secrets where no OS keychain exists ([#1950](https://github.com/modelcontextprotocol/inspector/issues/1950)), +paste-MCP-JSON ([#904](https://github.com/modelcontextprotocol/inspector/issues/904)), and registry discovery ([#1101](https://github.com/modelcontextprotocol/inspector/issues/1101)). + +### 4.10 Workspace and layout + +Multiple servers side by side — the actual shape of debugging a gateway, or comparing a +server against a reference implementation. Detachable/resizable panels, remembered layout per +server, density modes, and full-collapse ([#928](https://github.com/modelcontextprotocol/inspector/issues/928)). Prerequisite for cross-server timeline +correlation. + +### 4.11 Performance at scale + +A 1000-tool server or a long-running session should not degrade. Virtualize the long lists and +logs; cap in-memory protocol history with spill-to-disk; truncate large payloads by default +with explicit expansion (which is also the right default for reference results, §3.6). + +### 4.12 Accessibility and keyboard-first operation + +Full keyboard operation across every tab, correct roles and labels, high-contrast support, +and `prefers-reduced-motion` (which the timeline's animations will make newly relevant). We +have a Storybook a11y harness already; the gap is coverage, not tooling. + +### 4.13 Onboarding + +A first run currently presents an empty server list and no path forward. Add a guided first +connection, one-click example servers drawn from `test-servers/`, and inline links from each +panel to the relevant spec section. + +### 4.14 Plugin architecture + +[#1025](https://github.com/modelcontextprotocol/inspector/issues/1025). The multiplier on everything above — custom panels, custom transports (§3.1), +interceptor hosting (§3.7), and community-contributed views without core changes. Sequenced +late deliberately: designing a plugin API before the timeline, diff, and session format exist +would mean designing it against the wrong surfaces. + +--- + +## 5. Sequencing + +Four phases of roughly six weekly milestones each. Track A items appear where their upstream +signal is expected; Track B items are placed to unblock Track A wherever possible. + +### Phase 1 — Foundations (~`v2.2` – `v2.7`, Aug–Sep 2026) + +_Build the general surfaces the rest of the plan renders into, and clear the debt that makes +first impressions bad._ + +- 🅑 **Zoomable timeline v1** — lanes, spans, zoom/pan, click-through +- 🅑 **Argument editor workstream** (§4.7) — closes six issues as one +- 🅑 **Connection Doctor** (§4.8) + the local-host connection fixes +- 🅐 `Last-Event-ID` resumption ([#920](https://github.com/modelcontextprotocol/inspector/issues/920)); `Mcp-Name` on Tasks ([#1917](https://github.com/modelcontextprotocol/inspector/issues/1917)); discover checkmarks ([#1887](https://github.com/modelcontextprotocol/inspector/issues/1887)) +- 🅐 `server.json` support ([#922](https://github.com/modelcontextprotocol/inspector/issues/922)) — prerequisite for Server Cards +- ⚙️ Windows CI/gate fixes already in `v2.2.0` + +### Phase 2 — Artifacts and comparison (~`v2.8` – `v2.13`, Sep–Nov 2026) + +_Make sessions into things you can keep, share, and compare._ + +- 🅑 **Session record / replay / share** (§4.2) — format shared with audit transcript +- 🅑 **Diff primitive** (§4.3) — then wire capability diff ([#1034](https://github.com/modelcontextprotocol/inspector/issues/1034)) +- 🅑 **Command palette and global search** (§4.4) +- 🅐 **OTLP export and audit transcript** (§3.4) — no upstream dependency +- 🅐 **Grouped sidebars** (§3.10) on client-side heuristics +- 🅐 Strict schema validation ([#1005](https://github.com/modelcontextprotocol/inspector/issues/1005), [#1015](https://github.com/modelcontextprotocol/inspector/issues/1015)) +- 🅐 Timeline lanes for tasks and sessions (falls out of Phase 1) + +### Phase 3 — Automation and spec catch-up (~`v2.14` – `v2.20`, Nov 2026 – Jan 2027) + +_Turn the Inspector into something you can run in CI, and absorb the SEPs that have landed._ + +- 🅑 **Saved calls / collections** (§4.5) → **assertions and CI flows** (§4.6) +- 🅐 **Conformance runner** (§3.11) — shares the assertion engine +- 🅐 **File uploads** (§3.8) — assumes the TS SDK reference impl has shipped +- 🅐 **Server Card preview + card-vs-reality diff** (§3.2) — assumes SEP-2127 has settled +- 🅐 **Skills view** (§3.9) — assumes SEP-2640 accepted +- 🅑 Performance at scale (§4.11); accessibility pass (§4.12) + +### Phase 4 — Frontier (~`v2.21` – `v2.27`, Jan–Feb 2027) + +_The items whose shape we cannot yet commit to, plus the multiplier._ + +- 🅐 **Interceptor test bench** (§3.7) — and a decision on owning the WG's CLI deliverable +- 🅐 **Triggers/events receiver** (§3.5) — design throughout, build only if the SEP lands +- 🅐 **Transport/session work** (§3.1) — proxy harness, stateless verification, third era +- 🅐 **ID-JAG / Cross-App Access flow** (§3.4) +- 🅑 **Plugin architecture** (§4.14) — designed against surfaces that now exist +- 🅑 Workspace and layout (§4.10); onboarding (§4.13) + +### Standing commitments across all phases + +- **Weekly milestone cadence** and the `npm run ci` gate are unchanged. +- **Bug and triage capacity is reserved, not scheduled.** The board's Incoming queue keeps + flowing regardless of phase. +- **WG liaison**: attend Transports, Agents, Triggers, Interceptors, and Server Card sessions + and feed implementation experience back. Several items above are as much _inputs to_ the + spec as outputs of it. + +--- + +## 6. What we are deliberately not doing + +Stating these so they are decisions rather than oversights. + +- **Not building bespoke panels per SEP.** Where a new feature can render into the timeline, + the diff, or the session format, it does. A new top-level tab needs justification. +- **Not chasing pre-Draft SEPs.** 🔴 items get a tracking issue and a WG liaison, not code. + We were burned by this in v1. +- **Not publishing `core/` as a package this cycle.** [#1636](https://github.com/modelcontextprotocol/inspector/issues/1636) stays deferred; it adds an API + compatibility obligation we cannot yet afford. +- **Not adding transports beyond what the spec blesses**, per the roadmap — but §3.1 makes + _custom_ transports loadable so the community can experiment. +- **Not building a second extension mechanism.** If we host interceptors, they run on the + plugin architecture (§4.14). + +--- + +## 7. Open questions + +For WG discussion before this plan is adopted. + +1. **Does the private roadmap doc change §3?** This plan is built from the public roadmap; the + private doc may carry timelines or themes it omits. +2. **Do we claim the Interceptors WG's "CLI client for interceptor invocation and testing"?** + It is Ideating and unowned, it describes our CLI, and we have a co-lead in common. If yes, + it needs milestone allocation in Phase 3, not Phase 4. +3. **How far do we take the conformance role?** §3.11 and §4.6 point at "the Inspector tells + you whether your server is correct." That is a real expansion of mission — worth an + explicit yes or no, and possibly a charter amendment. +4. **Who owns the triggers/events reachability problem?** A publicly reachable callback + endpoint on a localhost dev tool is a security question as much as a UX one, and it needs + an owner before Phase 4. +5. **Is the ~50/50 capacity split right?** It is an assertion in this draft, not a measurement. +6. **Timeline v1 scope.** The §4.1 sketch is deliberately broad. Which parts are v1 and which + are follow-ups should be settled before Phase 1 starts. + +--- + +## 8. Sources + +- [MCP Roadmap](https://modelcontextprotocol.io/development/roadmap) (last updated 2026-03-05) +- WG charters: [Inspector V2](https://modelcontextprotocol.io/community/working-groups/inspector-v2) · [Server Card](https://modelcontextprotocol.io/community/working-groups/server-card) · [Triggers & Events](https://modelcontextprotocol.io/community/working-groups/triggers-events) · [Agents](https://modelcontextprotocol.io/community/working-groups/agents) · [Interceptors](https://modelcontextprotocol.io/community/working-groups/interceptors) · [File Uploads](https://modelcontextprotocol.io/community/working-groups/file-uploads) · [Skills Over MCP](https://modelcontextprotocol.io/community/working-groups/skills-over-mcp) +- IG charters: [Primitive Grouping](https://modelcontextprotocol.io/community/interest-groups/primitive-grouping) · [Tool Annotations](https://modelcontextprotocol.io/community/interest-groups/tool-annotations) · [Enterprise-Managed Authorization](https://modelcontextprotocol.io/community/interest-groups/enterprise-managed-authorization) +- Internal: [`specification/v2_new_spec_impact.md`](../specification/v2_new_spec_impact.md) · [`specification/v2_scope.md`](../specification/v2_scope.md) · [`specification/v2_ux_features.md`](../specification/v2_ux_features.md) +- [Inspector V2 project board (#28)](https://github.com/orgs/modelcontextprotocol/projects/28) From cf834633ba99a569ae766083bde166646fc13ac6 Mon Sep 17 00:00:00 2001 From: olaservo Date: Wed, 12 Aug 2026 08:56:24 -0700 Subject: [PATCH 03/25] fix: add eol=lf policy to .gitattributes so Windows checkouts stay clean Co-Authored-By: Claude Fable 5 --- .gitattributes | 1 + 1 file changed, 1 insertion(+) 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 From adde9ccf948a3d23fff93270be6c53188f4d5d0b Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 15 Aug 2026 12:13:06 -0400 Subject: [PATCH 04/25] fix: bind loopback by address and pin the sandbox port so containers work MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defaults that each break the Inspector inside a container. #1951 — the web UI never loads in a VS Code dev container. `HOST` defaulted to the NAME `localhost`, and `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 is `::1`. The server bound IPv6 loopback only and refused every IPv4 client, including the dev container port forwarder. It presents as an endless spinner rather than a connection error because the forwarder accepts on the host side before failing to connect inward. Default to the ADDRESS 127.0.0.1 instead; an explicit HOST=localhost is still honored as typed. #2008 — the Apps tab is unreachable in any container. The MCP Apps sandbox listened on an OS-assigned port, so it changed every run and could never be named in a devcontainer `forwardPorts`, a `docker run -p`, or a tunnel. Default it to a fixed 6275, mirroring CLIENT_PORT's fixed 6274, and retry once on an OS-assigned port (loudly) if it is taken — the collision a dynamic default never had. Verified in the reporter's exact image (mcr.microsoft.com/devcontainers/typescript-node:4-24-trixie): both listeners come up on 127.0.0.1 and answer 200 on 6274 and 6275/sandbox. Also EXPOSE 6275 and set MCP_SANDBOX_PORT in the image, and document both defaults in the web README, the migration guide, and the root README's Docker section. Signed-off-by: cliffhall --- AGENTS.md | 5 +- Dockerfile | 6 +- README.md | 9 ++ clients/web/README.md | 6 +- clients/web/server/resolve-bind-host.ts | 43 ++++-- clients/web/server/sandbox-controller.ts | 61 ++++++++- .../server/resolve-bind-host.test.ts | 16 ++- .../server/sandbox-controller.test.ts | 127 +++++++++++++++--- .../server/web-server-config.test.ts | 21 +-- docs/v1-to-v2-migration.md | 8 +- 10 files changed, 248 insertions(+), 54 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 21061009..4123ccad 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), 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..35b1ce6d 100644 --- a/README.md +++ b/README.md @@ -349,6 +349,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/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/test/integration/server/resolve-bind-host.test.ts b/clients/web/src/test/integration/server/resolve-bind-host.test.ts index c6de0ada..691230a1 100644 --- a/clients/web/src/test/integration/server/resolve-bind-host.test.ts +++ b/clients/web/src/test/integration/server/resolve-bind-host.test.ts @@ -1,12 +1,24 @@ import { describe, it, expect } from "vitest"; import { BIND_ALL_INTERFACES_ENV, + DEFAULT_BIND_HOST, resolveBindHostname, } from "../../../../server/resolve-bind-host.js"; describe("resolveBindHostname", () => { - it("defaults to localhost when HOST is unset", () => { - expect(resolveBindHostname({})).toBe("localhost"); + // Regression guard for #1951. The default must be an *address*, never the + // name `localhost`: `listen()` resolves a name and binds the single address + // the resolver returns first, which on glibc Linux is `::1` — leaving every + // IPv4 client (a dev container's port forwarder, an `ssh -L` to 127.0.0.1, a + // container healthcheck) refused. See DEFAULT_BIND_HOST. + it("defaults to the IPv4 loopback address, not the name localhost", () => { + expect(resolveBindHostname({})).toBe("127.0.0.1"); + expect(resolveBindHostname({})).toBe(DEFAULT_BIND_HOST); + expect(resolveBindHostname({})).not.toBe("localhost"); + }); + + it("still honors an explicit HOST=localhost as typed", () => { + expect(resolveBindHostname({ HOST: "localhost" })).toBe("localhost"); }); it("returns a loopback HOST unchanged", () => { diff --git a/clients/web/src/test/integration/server/sandbox-controller.test.ts b/clients/web/src/test/integration/server/sandbox-controller.test.ts index 5db8d1b3..a1f73f7e 100644 --- a/clients/web/src/test/integration/server/sandbox-controller.test.ts +++ b/clients/web/src/test/integration/server/sandbox-controller.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import { createServer, type Server } from "node:http"; import { createSandboxController, + DEFAULT_SANDBOX_PORT, resolveSandboxPort, sandboxFrameAncestors, } from "../../../../server/sandbox-controller.js"; @@ -75,7 +76,17 @@ describe("resolveSandboxPort", () => { else process.env.SERVER_PORT = envSnapshot.server; }); - it("returns 0 when no env vars are set", () => { + // Regression guard for #2008: the default must be a FIXED port. An + // OS-assigned one changes every run, so it can never be named in a dev + // container's `forwardPorts`, a `docker run -p`, or an SSH tunnel — leaving + // the Apps tab unreachable in every such environment. + it("returns a fixed default port when no env vars are set", () => { + expect(resolveSandboxPort()).toBe(DEFAULT_SANDBOX_PORT); + expect(resolveSandboxPort()).not.toBe(0); + }); + + it("still honors an explicit MCP_SANDBOX_PORT=0 as OS-assigned", () => { + process.env.MCP_SANDBOX_PORT = "0"; expect(resolveSandboxPort()).toBe(0); }); @@ -102,19 +113,19 @@ describe("resolveSandboxPort", () => { expect(resolveSandboxPort()).toBe(9100); }); - it("returns 0 when SERVER_PORT is non-numeric and no MCP_SANDBOX_PORT", () => { + it("falls back to the default when SERVER_PORT is non-numeric and no MCP_SANDBOX_PORT", () => { process.env.SERVER_PORT = "not-a-port"; - expect(resolveSandboxPort()).toBe(0); + expect(resolveSandboxPort()).toBe(DEFAULT_SANDBOX_PORT); }); it("ignores empty-string SERVER_PORT", () => { process.env.SERVER_PORT = ""; - expect(resolveSandboxPort()).toBe(0); + expect(resolveSandboxPort()).toBe(DEFAULT_SANDBOX_PORT); }); it("ignores negative values", () => { process.env.MCP_SANDBOX_PORT = "-1"; - expect(resolveSandboxPort()).toBe(0); + expect(resolveSandboxPort()).toBe(DEFAULT_SANDBOX_PORT); }); it("rejects a partial-parse value (6274abc) rather than binding 6274", () => { @@ -125,7 +136,7 @@ describe("resolveSandboxPort", () => { it("rejects an out-of-range value (70000) so it can't crash listen", () => { process.env.MCP_SANDBOX_PORT = "70000"; - expect(resolveSandboxPort()).toBe(0); + expect(resolveSandboxPort()).toBe(DEFAULT_SANDBOX_PORT); }); }); @@ -135,7 +146,8 @@ describe("createSandboxController", () => { try { const { url, port } = await controller.start(); expect(port).toBeGreaterThan(0); - expect(url).toBe(`http://localhost:${port}/sandbox`); + // Default host is now the IPv4 loopback address, not the name (#1951). + expect(url).toBe(`http://127.0.0.1:${port}/sandbox`); expect(controller.getUrl()).toBe(url); const res = await fetch(url); @@ -312,11 +324,11 @@ describe("createSandboxController", () => { } }); - it("resolves with empty values when listen fails (EADDRINUSE)", async () => { - // Bind a placeholder HTTP server to claim a port, then point a sandbox - // controller at the same port to force EADDRINUSE. The Vite plugin awaits - // start() in configureServer; if start() ever stops resolving, the entire - // dev backend hangs. This test pins down the resolve-on-error contract. + /** Claim a port so a subsequent listen on it fails with EADDRINUSE. */ + async function claimPort(): Promise<{ + port: number; + release: () => Promise; + }> { const blocker: Server = createServer(); await new Promise((resolve) => blocker.listen(0, "127.0.0.1", () => resolve()), @@ -327,21 +339,96 @@ describe("createSandboxController", () => { ? addr.port : 0; expect(port).toBeGreaterThan(0); + return { + port, + release: () => + new Promise((resolve) => blocker.close(() => resolve())), + }; + } + it("falls back to an OS-assigned port when the fixed port is taken", async () => { + // #2008 made the default port fixed, which introduced a collision the + // dynamic default never had: a second Inspector, or anything else already + // on 6275, would have taken the Apps tab down entirely. Retrying on a + // dynamic port keeps it working locally. + const { port, release } = await claimPort(); + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + const controller = createSandboxController({ port, host: "127.0.0.1" }); + try { + const result = await controller.start(); + expect(result.port).toBeGreaterThan(0); + expect(result.port).not.toBe(port); + expect(result.url).toBe(`http://127.0.0.1:${result.port}/sandbox`); + expect(controller.getUrl()).toBe(result.url); + // The sandbox must actually be serving on the fallback port, not merely + // reporting one. + expect((await fetch(result.url)).status).toBe(200); + // Loud, not silent: a pinned-and-forwarded port that quietly moved is the + // very failure the fixed default exists to prevent. + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining(`Sandbox: port ${port} in use`), + ); + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining("MCP_SANDBOX_PORT"), + ); + } finally { + warnSpy.mockRestore(); + await controller.close(); + await release(); + } + }); + + it("resolves with empty values when even the dynamic retry fails", async () => { + // The Vite plugin awaits start() in configureServer; if start() ever stops + // resolving, the entire dev backend hangs. This pins the resolve-on-error + // contract for the case the retry can't rescue — an explicit `port: 0` that + // fails has nowhere left to fall back to, so it degrades as before. + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + const controller = createSandboxController({ port: 0, host: "127.0.0.1" }); + try { + // Force the listen to fail the way a dynamic listen can (an unusable bind + // address), since port 0 cannot collide. + const broken = createSandboxController({ + port: 0, + host: "203.0.113.1", // TEST-NET-3: not assigned to any local interface + }); + const result = await broken.start(); + expect(result).toEqual({ port: 0, url: "" }); + expect(broken.getUrl()).toBeNull(); + // close() must be a no-op since the server never bound. + await expect(broken.close()).resolves.toBeUndefined(); + expect(errorSpy).toHaveBeenCalledWith( + "Sandbox server error:", + expect.objectContaining({ code: "EADDRNOTAVAIL" }), + ); + } finally { + errorSpy.mockRestore(); + await controller.close(); + } + }); + + it("gives up after one retry when the dynamic fallback also fails", async () => { + // Guards the `retriedDynamic` latch: without it an EADDRINUSE loop could + // re-listen forever instead of settling. + const { port, release } = await claimPort(); + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); const controller = createSandboxController({ port, host: "127.0.0.1" }); try { const result = await controller.start(); - expect(result).toEqual({ port: 0, url: "" }); - expect(controller.getUrl()).toBeNull(); - // close() must be a no-op since the server never bound. - await expect(controller.close()).resolves.toBeUndefined(); - expect(errorSpy).toHaveBeenCalledWith( - expect.stringContaining(`Sandbox: port ${port} in use`), - ); + // The retry succeeds here (a dynamic port is always available), so assert + // the latch is what bounds it: exactly one retry warning was emitted. + expect( + warnSpy.mock.calls.filter((c) => + String(c[0]).includes("falling back to an OS-assigned port"), + ), + ).toHaveLength(1); + expect(result.port).toBeGreaterThan(0); } finally { + warnSpy.mockRestore(); errorSpy.mockRestore(); - await new Promise((resolve) => blocker.close(() => resolve())); + await controller.close(); + await release(); } }); diff --git a/clients/web/src/test/integration/server/web-server-config.test.ts b/clients/web/src/test/integration/server/web-server-config.test.ts index 5436b8fe..7dbb2e4a 100644 --- a/clients/web/src/test/integration/server/web-server-config.test.ts +++ b/clients/web/src/test/integration/server/web-server-config.test.ts @@ -7,6 +7,7 @@ import { webServerConfigToInitialPayload, type WebServerConfig, } from "../../../../server/web-server-config.js"; +import { DEFAULT_SANDBOX_PORT } from "../../../../server/sandbox-controller.js"; import { API_SERVER_ENV_VARS, LEGACY_AUTH_TOKEN_ENV, @@ -32,7 +33,7 @@ const MUTATED_ENV_KEYS = [ const baseConfig = (): WebServerConfig => ({ port: 6274, - hostname: "localhost", + hostname: "127.0.0.1", authToken: "tok", dangerouslyOmitAuth: false, initialMcpConfig: null, @@ -42,7 +43,7 @@ const baseConfig = (): WebServerConfig => ({ storageDir: undefined, allowedOrigins: ["http://localhost:6274"], sandboxPort: 0, - sandboxHost: "localhost", + sandboxHost: "127.0.0.1", logger: undefined, autoOpen: false, }); @@ -72,7 +73,7 @@ describe("buildWebServerConfigFromEnv", () => { it("returns defaults when no env vars are set", () => { const cfg = buildWebServerConfigFromEnv(); expect(cfg.port).toBe(6274); - expect(cfg.hostname).toBe("localhost"); + expect(cfg.hostname).toBe("127.0.0.1"); expect(cfg.authToken).toBe(""); expect(cfg.dangerouslyOmitAuth).toBe(false); expect(cfg.initialMcpConfig).toBeNull(); @@ -82,8 +83,8 @@ describe("buildWebServerConfigFromEnv", () => { "http://127.0.0.1:6274", "http://[::1]:6274", ]); - expect(cfg.sandboxPort).toBe(0); - expect(cfg.sandboxHost).toBe("localhost"); + expect(cfg.sandboxPort).toBe(DEFAULT_SANDBOX_PORT); + expect(cfg.sandboxHost).toBe("127.0.0.1"); expect(cfg.logger).toBeUndefined(); // Vitest sets `process.env.VITEST = 'true'`, so the autoOpen default is // suppressed here. Real `vite dev` runs don't set VITEST and default to @@ -289,7 +290,9 @@ describe("buildWebServerConfigFromEnv", () => { it("ignores non-numeric SERVER_PORT", () => { process.env.SERVER_PORT = "nope"; - expect(buildWebServerConfigFromEnv().sandboxPort).toBe(0); + expect(buildWebServerConfigFromEnv().sandboxPort).toBe( + DEFAULT_SANDBOX_PORT, + ); }); it("treats MCP_SANDBOX_PORT empty string as unset", () => { @@ -596,7 +599,7 @@ describe("printServerBanner", () => { it("includes the auth token in the URL when auth is enabled", () => { const url = printServerBanner(baseConfig(), 6274, "secret", undefined); expect(url).toBe( - `http://localhost:6274?${API_SERVER_ENV_VARS.AUTH_TOKEN}=secret`, + `http://127.0.0.1:6274?${API_SERVER_ENV_VARS.AUTH_TOKEN}=secret`, ); expect(logSpy.lines.some((l) => l.includes("Auth token: secret"))).toBe( true, @@ -607,7 +610,7 @@ describe("printServerBanner", () => { const cfg = baseConfig(); cfg.dangerouslyOmitAuth = true; const url = printServerBanner(cfg, 6274, "irrelevant", undefined); - expect(url).toBe("http://localhost:6274"); + expect(url).toBe("http://127.0.0.1:6274"); expect( logSpy.lines.some((l) => l.includes("Auth: disabled (DANGEROUSLY_OMIT_AUTH)"), @@ -617,7 +620,7 @@ describe("printServerBanner", () => { it("omits the query string when no token is supplied", () => { const url = printServerBanner(baseConfig(), 6274, "", undefined); - expect(url).toBe("http://localhost:6274"); + expect(url).toBe("http://127.0.0.1:6274"); }); it("brackets an IPv6 bind host in the printed URL", () => { diff --git a/docs/v1-to-v2-migration.md b/docs/v1-to-v2-migration.md index a2db3f55..0d1b2fe3 100644 --- a/docs/v1-to-v2-migration.md +++ b/docs/v1-to-v2-migration.md @@ -66,7 +66,7 @@ Consequences: - `MCP_PROXY_FULL_ADDRESS` has no v2 equivalent and is ignored. It existed to tell the browser where a non-default proxy lived; there is no proxy. - Docker needs `-p 6274:6274` for the UI, where the v1 recipe published `6277` as well. -⚠️ **`6274` is not the only listener.** The web backend also starts a **separate MCP Apps sandbox server**, on a dynamic port by default. It is only used by the Apps tab, and on plain loopback it needs no attention — but the browser reaches it directly, so anywhere the Inspector is _not_ served from the browser's own machine (Docker, a remote host, an SSH tunnel) you must pin it with `MCP_SANDBOX_PORT` and expose/forward that port too, or the Apps tab won't load. This is a different port from v1's proxy — it carries no MCP traffic — but it is still a second port. See [MCP Apps caveats](../clients/web/README.md#hosting-on-a-network). +⚠️ **`6274` is not the only listener.** The web backend also starts a **separate MCP Apps sandbox server**, on a fixed **`6275`** by default (it was OS-assigned before #2008). It is only used by the Apps tab, and on plain loopback it needs no attention — but the browser reaches it directly, so anywhere the Inspector is _not_ served from the browser's own machine (Docker, a dev container, a remote host, an SSH tunnel) you must expose/forward that port too, or the Apps tab won't load. This is a different port from v1's proxy — it carries no MCP traffic — but it is still a second port. See [MCP Apps caveats](../clients/web/README.md#hosting-on-a-network). ## Launching @@ -280,7 +280,7 @@ Stdout is otherwise compatible: the default `text` format still pretty-prints th | `MCP_AUTO_OPEN_ENABLED` | same | Also gates non-TTY interactive OAuth in the CLI/TUI | | — | `DANGEROUSLY_BIND_ALL_INTERFACES` | New opt-in for a wildcard bind (the Docker image sets it) | | — | `MCP_CATALOG_PATH` | Default catalog path | -| — | `MCP_SANDBOX_PORT` | MCP Apps sandbox server port (dynamic by default) | +| — | `MCP_SANDBOX_PORT` | MCP Apps sandbox server port (fixed `6275` by default) | | — | `MCP_STORAGE_DIR`, `MCP_INSPECTOR_OAUTH_STATE_PATH`, `MCP_CLIENT_CONFIG_PATH` | Storage and OAuth state locations | | — | `MCP_OAUTH_CALLBACK_URL` | CLI/TUI OAuth loopback callback | @@ -355,7 +355,9 @@ Notes: **"`Transport type not specified and could not be determined from URL`."** The path ends in neither `/mcp` nor `/sse`, so v2 refuses to guess where v1 fell back to SSE. Pass `--transport http` or `--transport sse`. -**"The Apps tab is blank in Docker / over SSH."** The MCP Apps sandbox is a second, dynamic port. Pin it with `MCP_SANDBOX_PORT` and publish/forward it too — see [MCP Apps caveats](../clients/web/README.md#hosting-on-a-network). +**"The Apps tab is blank in Docker / over SSH."** The MCP Apps sandbox is a second port (`6275` by default). Publish/forward it too — see [MCP Apps caveats](../clients/web/README.md#hosting-on-a-network). + +**"The page never loads inside a VS Code dev container (or any container)."** Fixed in the release carrying #1951 — upgrade. Before that fix the default `HOST=localhost` bound IPv6 loopback only on Linux, so the container's port forwarder (which connects over IPv4 `127.0.0.1`) could never reach it; the terminal printed a normal banner while the browser spun forever. On an older version, work around it with `HOST=127.0.0.1`. Note the failure is a **hang**, not a connection error, because the forwarder accepts on the host side before failing to connect inward. **"`HOST=0.0.0.0` exits with an error."** That's the wildcard-bind guard. Bind a specific address instead, or set `DANGEROUSLY_BIND_ALL_INTERFACES=true` if you genuinely need all interfaces. See [Host binding & the origin allow-list](../clients/web/README.md#host-binding--the-origin-allow-list). From 1ea1e30f6344985927cf68657bd583dbf7f928cf Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 15 Aug 2026 13:17:08 -0400 Subject: [PATCH 05/25] =?UTF-8?q?fix:=20address=20Copilot=20review=20?= =?UTF-8?q?=E2=80=94=20real=20latch=20coverage,=20consistent=20docker=20do?= =?UTF-8?q?cs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - The `retriedDynamic` latch test never made the retry fail: a real `listen(0)` always succeeds, so it produced one EADDRINUSE and passed with the latch deleted. Mock `node:http` so every listen fails, asserting exactly two listen attempts and a settled result. Verified by mutation: removing the latch now hangs the test instead of passing it. - The migration guide's Docker notes still described a dynamic sandbox port and told readers to pin a custom one, contradicting the new fixed 6275 two sections later. - Narrowed "or any container" in the troubleshooting entry, which overstated the scope and contradicted the PR's own scope note. It now names the IPv4-forwarder case and explicitly excludes the official image and a plain loopback -p. Signed-off-by: cliffhall --- .../server/sandbox-controller.test.ts | 58 ++++++++++++++++--- docs/v1-to-v2-migration.md | 9 +-- 2 files changed, 54 insertions(+), 13 deletions(-) diff --git a/clients/web/src/test/integration/server/sandbox-controller.test.ts b/clients/web/src/test/integration/server/sandbox-controller.test.ts index a1f73f7e..6614c06e 100644 --- a/clients/web/src/test/integration/server/sandbox-controller.test.ts +++ b/clients/web/src/test/integration/server/sandbox-controller.test.ts @@ -1,5 +1,6 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import { createServer, type Server } from "node:http"; +import { EventEmitter } from "node:events"; import { createSandboxController, DEFAULT_SANDBOX_PORT, @@ -408,27 +409,66 @@ describe("createSandboxController", () => { }); it("gives up after one retry when the dynamic fallback also fails", async () => { - // Guards the `retriedDynamic` latch: without it an EADDRINUSE loop could - // re-listen forever instead of settling. - const { port, release } = await claimPort(); + // Guards the `retriedDynamic` latch. A real `listen(0)` effectively always + // succeeds, so claiming a port only ever produces ONE EADDRINUSE — under + // which this test would pass with the latch deleted. Mock `node:http` so + // every listen fails with EADDRINUSE: the first trips the retry, the second + // must be caught by the latch and settle. Without the latch this recurses + // until the test times out rather than resolving. + vi.resetModules(); + const listenCalls: number[] = []; + vi.doMock("node:http", async () => { + const actual = + await vi.importActual("node:http"); + return { + ...actual, + createServer: () => { + const emitter = new EventEmitter(); + return Object.assign(emitter, { + listen: (p: number) => { + listenCalls.push(p); + setImmediate(() => + emitter.emit( + "error", + Object.assign(new Error("in use"), { code: "EADDRINUSE" }), + ), + ); + }, + address: () => null, + close: (cb?: () => void) => cb?.(), + }); + }, + }; + }); const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); - const controller = createSandboxController({ port, host: "127.0.0.1" }); try { + const mod = await import("../../../../server/sandbox-controller.js"); + const controller = mod.createSandboxController({ + port: 6275, + host: "127.0.0.1", + }); + // Settles rather than looping — the contract the Vite plugin depends on. const result = await controller.start(); - // The retry succeeds here (a dynamic port is always available), so assert - // the latch is what bounds it: exactly one retry warning was emitted. + expect(result).toEqual({ port: 0, url: "" }); + expect(controller.getUrl()).toBeNull(); + // Exactly two listens: the fixed port, then one dynamic retry. A third + // would mean the latch failed to bound the loop. + expect(listenCalls).toEqual([6275, 0]); expect( warnSpy.mock.calls.filter((c) => String(c[0]).includes("falling back to an OS-assigned port"), ), ).toHaveLength(1); - expect(result.port).toBeGreaterThan(0); + // The second failure degrades loudly instead of retrying again. + expect(errorSpy).toHaveBeenCalledWith( + expect.stringContaining("Sandbox: port 6275 in use"), + ); } finally { warnSpy.mockRestore(); errorSpy.mockRestore(); - await controller.close(); - await release(); + vi.doUnmock("node:http"); + vi.resetModules(); } }); diff --git a/docs/v1-to-v2-migration.md b/docs/v1-to-v2-migration.md index 0d1b2fe3..c241e9df 100644 --- a/docs/v1-to-v2-migration.md +++ b/docs/v1-to-v2-migration.md @@ -330,14 +330,15 @@ docker run --rm -p 127.0.0.1:6274:6274 ghcr.io/modelcontextprotocol/inspector:la Notes: - **Keep the `127.0.0.1:` prefix on the published port**, as the v1 recipe did. `-p 6274:6274` publishes on every host interface, and the container's `HOST=0.0.0.0` is about the _container's_ interfaces, not the host's — the two are independent. That matters here because the backend spawns processes, `/` embeds the API token, and requests arriving with **no** `Origin` header (i.e. anything that isn't a browser) skip the origin allow-list, leaving the token as the only guard. Bind to loopback and opt into wider exposure deliberately. -- **The Apps tab needs one more published port.** The sandbox server is dynamic by default and the `Dockerfile` `EXPOSE`s only `6274`, so the recipe above covers everything _except_ MCP Apps. To use them, pin and publish the sandbox port as well: +- **The Apps tab needs one more published port.** The sandbox server listens on `6275` by default, so the recipe above covers everything _except_ MCP Apps. To use them, publish that port as well: ```bash - docker run --rm -p 127.0.0.1:6274:6274 -p 127.0.0.1:6280:6280 \ - -e MCP_SANDBOX_PORT=6280 \ + docker run --rm -p 127.0.0.1:6274:6274 -p 127.0.0.1:6275:6275 \ ghcr.io/modelcontextprotocol/inspector:latest ``` + 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 advertises a port the browser can't reach. To use a different number, move both ends — `-e MCP_SANDBOX_PORT=6280 -p 127.0.0.1:6280:6280`. + - The v2 image sets `DANGEROUSLY_BIND_ALL_INTERFACES=true` internally (a container must bind `0.0.0.0` to be reachable through `-p`). Setting a bare `HOST=0.0.0.0` **outside** a container now exits with an error. - **If you remap the published port** (`-p 8080:6274`), the browser's origin no longer matches the in-container port, so set `ALLOWED_ORIGINS=http://localhost:8080,http://127.0.0.1:8080` (or run `-e CLIENT_PORT=8080 -p 8080:8080`) or connects will 403. - The image runs as non-root and has a `HEALTHCHECK` that assumes `--web`; add `--no-healthcheck` when running `--cli` / `--tui`. @@ -357,7 +358,7 @@ Notes: **"The Apps tab is blank in Docker / over SSH."** The MCP Apps sandbox is a second port (`6275` by default). Publish/forward it too — see [MCP Apps caveats](../clients/web/README.md#hosting-on-a-network). -**"The page never loads inside a VS Code dev container (or any container)."** Fixed in the release carrying #1951 — upgrade. Before that fix the default `HOST=localhost` bound IPv6 loopback only on Linux, so the container's port forwarder (which connects over IPv4 `127.0.0.1`) could never reach it; the terminal printed a normal banner while the browser spun forever. On an older version, work around it with `HOST=127.0.0.1`. Note the failure is a **hang**, not a connection error, because the forwarder accepts on the host side before failing to connect inward. +**"The page never loads inside a VS Code dev container."** Fixed in the release carrying #1951 — upgrade; on an older version, work around it with `HOST=127.0.0.1`. Before that fix the default `HOST=localhost` bound IPv6 loopback **only** on Linux, so anything connecting over IPv4 `127.0.0.1` was refused — which is what a dev container's port forwarder does. The terminal printed a normal banner while the browser spun forever; the failure is a **hang** rather than a connection error because the forwarder accepts on the host side before failing to connect inward. The same fix covers an `ssh -L 6274:127.0.0.1:6274` tunnel and a container healthcheck. It does **not** describe the official Docker image (it binds `0.0.0.0`, so it was never affected) or a plain `docker run -p` against a loopback bind, which is unreachable across the network namespace for an unrelated reason. **"`HOST=0.0.0.0` exits with an error."** That's the wildcard-bind guard. Bind a specific address instead, or set `DANGEROUSLY_BIND_ALL_INTERFACES=true` if you genuinely need all interfaces. See [Host binding & the origin allow-list](../clients/web/README.md#host-binding--the-origin-allow-list). From a9ed087f253d35d79b603e45bac2ea816773a7c0 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 15 Aug 2026 14:16:03 -0400 Subject: [PATCH 06/25] chore: sync v2/main to 2.2.0 and bump on v2/main before the milestone merge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit v2/main has been at 2.0.0 since the v2.0.0 release while main and npm `latest` are at 2.2.0. The bump was created on the milestone-merge branch, which is cut from main, so it only ever existed downstream of v2/main and nothing carried it back — `chore: 2.1.0` and `chore: 2.2.0` are both on main and neither is on v2/main. That is not cosmetic. A branch cut from a milestone-merge branch silently carries the bump into an unrelated PR — #2009 arrived with a 2.0.0 -> 2.2.0 diff on a container bugfix — and anything reading the version in development (readInspectorVersion, --version, GET /api/config) reported a version two releases old. Move the bump to v2/main, before the milestone merge, so it flows into main with the rest of the milestone's work and the branches never disagree. The release tag is applied to the resulting main commit instead. The publish workflow needs no change: its tag-vs-package.json assertion runs at the release commit on main, which still carries the bump. Deliberately not a back-merge of main into v2/main: main holds the whole pre-v2 v1 history (~230 commits v2/main lacks, retained through `chore: replace main's tree with v2`), so merging would graft all of it into the develop branch's log permanently to deliver a two-file change. Also sets v2/main to 2.2.0 as a one-time catch-up. Nothing in the repo asserts a specific version value, so this is a safe two-file change. Closes #2010 Signed-off-by: cliffhall --- AGENTS.md | 4 ++++ README.md | 24 +++++++++++++++++++++--- package-lock.json | 4 ++-- package.json | 2 +- 4 files changed, 28 insertions(+), 6 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 4123ccad..9ac732b8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -465,6 +465,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 and the two branches never disagree about the version; the release tag is then applied to the resulting **`main`** commit. 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/README.md b/README.md index 35b1ce6d..9c9f59ee 100644 --- a/README.md +++ b/README.md @@ -326,16 +326,34 @@ 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: ```bash -npm version # bumps the root package.json + tags -git push --follow-tags +git checkout -b v2/chore/-bump-x-y-z v2/main +npm version --no-git-tag-version # 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 `main` and `v2/main` agree on the version at every point. + +**3. Tag the `main` commit and draft the Release:** + +```bash +git checkout main && git pull +git tag v && git push origin v # then draft & publish a GitHub Release for that tag → triggers `publish` ``` 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. diff --git a/package-lock.json b/package-lock.json index 5b738bc6..bb4866ac 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@modelcontextprotocol/inspector", - "version": "2.0.0", + "version": "2.2.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@modelcontextprotocol/inspector", - "version": "2.0.0", + "version": "2.2.0", "hasInstallScript": true, "license": "MIT", "dependencies": { diff --git a/package.json b/package.json index b255bb28..4e0295ac 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@modelcontextprotocol/inspector", - "version": "2.0.0", + "version": "2.2.0", "description": "The Model Context Protocol Inspector", "keywords": [ "MCP", From 9048e69758134145214d0bd8f1229ba6e7d9ffe6 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 15 Aug 2026 14:39:30 -0400 Subject: [PATCH 07/25] docs: drop the v prefix from the release tag step This repo's release tags are bare x.y.z (2.2.0, 2.1.0, 2.0.0), not v-prefixed. Also notes why the manual tag matters: npm's tag-version-prefix defaults to 'v' and the repo sets no .npmrc, so a bare `npm version` would have produced a tag that doesn't match the convention. Signed-off-by: cliffhall --- README.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 9c9f59ee..f90ed822 100644 --- a/README.md +++ b/README.md @@ -344,10 +344,12 @@ npm version --no-git-tag-version # bump only, no tag ```bash git checkout main && git pull -git tag v && git push origin v +git tag && git push origin # then draft & publish a GitHub Release for that tag → triggers `publish` ``` +⚠️ **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. From 4d567c97e4555c357e010340287b26aa2a489c05 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 15 Aug 2026 14:47:33 -0400 Subject: [PATCH 08/25] docs: make the release commands copy-pasteable and correct the invariant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review: - `` / `` inside a bash block is shell redirection, so both documented commands were syntax errors if copied. Verified: `bash -n` rejects `git tag && ...` and accepts `git tag 2.3.0 && ...`. Use a concrete 2.2.0 -> 2.3.0 example and say what to substitute. - "agree on the version at every point" / "never disagree" overstated it. The procedure deliberately bumps v2/main first, so between the bump and the milestone merge the branches differ — v2/main reads the version being built, main the one released. Describe the real invariant: they converge when the release merge lands, and v2/main is never left behind main. Signed-off-by: cliffhall --- AGENTS.md | 2 +- README.md | 12 ++++++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 9ac732b8..0baea459 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -465,7 +465,7 @@ 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 and the two branches never disagree about the version; the release tag is then applied to the resulting **`main`** commit. 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). + ⚠️ **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. diff --git a/README.md b/README.md index f90ed822..c845c540 100644 --- a/README.md +++ b/README.md @@ -330,21 +330,25 @@ Because there is **one version number** (only the root `package.json` has one **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 -git checkout -b v2/chore/-bump-x-y-z v2/main -npm version --no-git-tag-version # bump only, no tag +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 `main` and `v2/main` agree on the version at every point. +**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 checkout main && git pull -git tag && git push origin +git tag 2.3.0 && git push origin 2.3.0 # then draft & publish a GitHub Release for that tag → triggers `publish` ``` From d0143ef8437fc82b9755c268cd16a72b492dbfc8 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 15 Aug 2026 15:04:34 -0400 Subject: [PATCH 09/25] docs: tag origin/main explicitly so a release can't point at a local commit git checkout main && git pull resolves through the maintainer's configured merge/rebase strategy, so a divergent local main can produce or replay local commits. Tagging HEAD there tags a commit that is not on origin/main, and pushing only the tag leaves a release whose commit was never published. git tag origin/main pins the tag to what the remote branch actually points at, regardless of local state. Signed-off-by: cliffhall --- README.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index c845c540..a15210d1 100644 --- a/README.md +++ b/README.md @@ -347,11 +347,13 @@ Between steps 1 and 2 the two branches **do** differ, and that is expected, not **3. Tag the `main` commit and draft the Release:** ```bash -git checkout main && git pull -git tag 2.3.0 && git push origin 2.3.0 +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. From 937f1dd51b7310798265f3b303305ca3fe134a67 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 15 Aug 2026 16:10:17 -0400 Subject: [PATCH 10/25] fix: render nullable enum fields as a Select instead of a raw JSON textarea MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A tool argument that is optional AND explicitly nullable — Zod's `.nullish()` / `.nullable().optional()` — compiles to `anyOf: [{ type: "string", enum: [...] }, { type: "null" }]`. Every branch of the tool-call form's field dispatcher tests a single `type` string, so such a field matched nothing and fell through to the raw-JSON fallback, whose onChange stores the unparseable in-progress text back into state and re-`JSON.stringify`s it on the next render — compounding a fresh layer of escaping on every keystroke. `normalizeUnionType` already existed for the FastMCP `string|null` shape, but it only copied the surviving branch's `type` and dropped everything else, so the `enum` never reached the dispatcher; and nothing called it on a tool input schema — only elicitation requests, via `resolveRefsInMessage`. So: - Rewrite `normalizeUnionType` to hoist the non-null branch's own keywords (`enum`, `items`, `properties`, `minimum`, …) onto the flattened schema, as v1.x did. This also collapses nine near-identical blocks into two and picks up the `object`/`array` cases the `type: [X, "null"]` form was missing. - Normalize each field in `SchemaForm.renderField`, so tool, elicitation, and app forms all get it rather than just elicitation. - Give a nullable enum `Select` a clear affordance, so a value can be set back to `null`; a non-nullable enum keeps none, since it would reject it. Adds a `record_shipment` preset and `nullable-fields-http.json` showcase config serving all four nullable scalar shapes, for manual verification. Signed-off-by: cliffhall --- README.md | 7 + .../groups/SchemaForm/SchemaForm.test.tsx | 176 ++++++++++++++++++ .../groups/SchemaForm/SchemaForm.tsx | 21 ++- clients/web/src/utils/schemaUtils.test.ts | 114 ++++++++++++ clients/web/src/utils/schemaUtils.ts | 146 +++++++-------- .../configs/nullable-fields-http.json | 11 ++ test-servers/src/preset-registry.ts | 4 + test-servers/src/test-server-fixtures.ts | 33 ++++ 8 files changed, 428 insertions(+), 84 deletions(-) create mode 100644 test-servers/configs/nullable-fields-http.json diff --git a/README.md b/README.md index a15210d1..82164a98 100644 --- a/README.md +++ b/README.md @@ -145,6 +145,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 +228,12 @@ 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. + #### 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`. diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx index 5b9f064a..3ff41b50 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx @@ -811,3 +811,179 @@ 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(); + }); + + 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" } }); + }); + + 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"); + }); +}); diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx index 232b5327..eadb0564 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx @@ -12,6 +12,7 @@ import { useState } from "react"; import { ClearButton } from "../../elements/ClearButton/ClearButton"; import { useValueChange } from "../../../hooks/useValueChange"; import type { InspectorFormSchema } from "../../../utils/jsonUtils"; +import { normalizeUnionType } from "../../../utils/schemaUtils"; const FieldLabel = Text.withProps({ fw: 500, @@ -51,6 +52,16 @@ function toEnumData( return values; } +/** + * 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 @@ -207,7 +218,13 @@ export function SchemaForm({ 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 = normalizeUnionType(rawSchema); const isRequired = requiredFields.includes(fieldName); const label = fieldSchema.title ?? fieldName; const description = fieldSchema.description; @@ -223,6 +240,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)} /> @@ -243,6 +261,7 @@ export function SchemaForm({ withAsterisk={isRequired} disabled={disabled} data={data} + clearable={isClearable(fieldSchema)} value={(rawValue as string) ?? null} onChange={(val) => handleFieldChange(fieldName, val)} /> diff --git a/clients/web/src/utils/schemaUtils.test.ts b/clients/web/src/utils/schemaUtils.test.ts index 2689f317..59623fdb 100644 --- a/clients/web/src/utils/schemaUtils.test.ts +++ b/clients/web/src/utils/schemaUtils.test.ts @@ -347,6 +347,120 @@ describe("normalizeUnionType", () => { }; expect(normalizeUnionType(schema)).toBe(schema); }); + + it("ignores a two-member anyOf with no null branch", () => { + const schema = { + anyOf: [{ type: "string" as const }, { type: "number" as const }], + }; + expect(normalizeUnionType(schema)).toBe(schema); + }); + + it("ignores an anyOf whose non-null branch has no renderable type (#1928)", () => { + const schema = { + anyOf: [{ const: "only" }, { type: "null" as const }], + }; + expect(normalizeUnionType(schema)).toBe(schema); + }); + + // #1928: the enum lives on the surviving branch, and hoisting it is what makes + // the field render as a Select instead of the raw-JSON fallback. + it("hoists enum out of an anyOf string-enum|null branch", () => { + expect( + normalizeUnionType({ + description: "Direction", + anyOf: [ + { type: "string", enum: ["envio", "recebimento"] }, + { type: "null" }, + ], + }), + ).toEqual({ + type: "string", + description: "Direction", + enum: ["envio", "recebimento"], + anyOf: undefined, + nullable: true, + }); + }); + + it("infers string for a typeless enum branch", () => { + expect( + normalizeUnionType({ + anyOf: [{ enum: ["a", "b"] }, { type: "null" }], + }), + ).toEqual({ + type: "string", + enum: ["a", "b"], + anyOf: undefined, + nullable: true, + }); + }); + + it("hoists items out of an anyOf array|null branch", () => { + expect( + normalizeUnionType({ + anyOf: [ + { type: "array", items: { type: "string", enum: ["a", "b"] } }, + { type: "null" }, + ], + }), + ).toEqual({ + type: "array", + items: { type: "string", enum: ["a", "b"] }, + anyOf: undefined, + nullable: true, + }); + }); + + it("hoists properties out of an anyOf object|null branch", () => { + expect( + normalizeUnionType({ + anyOf: [ + { type: "object", properties: { a: { type: "string" } } }, + { type: "null" }, + ], + }), + ).toEqual({ + type: "object", + properties: { a: { type: "string" } }, + anyOf: undefined, + nullable: true, + }); + }); + + it("normalizes anyOf object|null regardless of branch order", () => { + expect( + normalizeUnionType({ + anyOf: [{ type: "null" }, { type: "object" }], + }), + ).toEqual({ type: "object", anyOf: undefined, nullable: true }); + }); + + it("normalizes type array object|null", () => { + expect( + normalizeUnionType({ + type: ["object", "null"], + properties: { a: { type: "string" } }, + }), + ).toEqual({ + type: "object", + properties: { a: { type: "string" } }, + nullable: true, + }); + }); + + it("normalizes type array array|null", () => { + expect(normalizeUnionType({ type: ["array", "null"] })).toEqual({ + type: "array", + nullable: true, + }); + }); + + it("keeps a type array of two non-null members unchanged", () => { + const schema = { type: ["string", "number"] as const }; + expect(normalizeUnionType({ ...schema, type: [...schema.type] })).toEqual({ + type: ["string", "number"], + }); + }); }); describe("formatFieldLabel", () => { diff --git a/clients/web/src/utils/schemaUtils.ts b/clients/web/src/utils/schemaUtils.ts index 4e578a1c..2624fba1 100644 --- a/clients/web/src/utils/schemaUtils.ts +++ b/clients/web/src/utils/schemaUtils.ts @@ -201,101 +201,81 @@ export function resolveRef( } /** - * Normalizes union types (like string|null from FastMCP) to simple types for form rendering + * The `type` values {@link SchemaForm} dispatches on. `"null"` is deliberately + * absent: it is the branch a nullable union is being flattened *away* from, and + * there is no widget for a field whose only permitted value is `null`. + */ +const RENDERABLE_TYPES = [ + "string", + "number", + "integer", + "boolean", + "array", + "object", +] as const; + +type RenderableType = (typeof RENDERABLE_TYPES)[number]; + +function isRenderableType(type: unknown): type is RenderableType { + return RENDERABLE_TYPES.includes(type as RenderableType); +} + +/** + * Normalizes a nullable union (`string|null` from FastMCP, or anything Zod's + * `.nullish()` / `.nullable()` compiles to) into the plain type the form + * renderer dispatches on. + * + * Two encodings mean the same thing and are both flattened to + * `{ type: , nullable: true }`: + * + * - `anyOf: [, { type: "null" }]` — the branch's *own* keywords are + * hoisted onto the result, because that is where the detail the renderer + * needs lives. A nullable enum compiles to + * `anyOf: [{ type: "string", enum: [...] }, { type: "null" }]`, so hoisting + * `enum` is what makes it a `Select` instead of falling through to the raw + * JSON fallback (#1928). The branch also wins over the wrapper on any shared + * key, matching v1.x's behavior. + * - `type: [, "null"]` — the keywords already sit at the top level, so only + * `type` collapses. + * + * Anything else — a union of two real types, a three-member `anyOf`, a branch + * whose type has no widget — is returned untouched and renders through the + * JSON fallback, which is the honest representation of a shape the form cannot + * model. + * * @param schema The JSON schema to normalize - * @returns A normalized schema or the original schema + * @returns A normalized schema, or the original schema when no nullable-union + * pattern matches */ export function normalizeUnionType( schema: InspectorFormSchema, ): InspectorFormSchema { - // Handle anyOf with exactly string and null (FastMCP pattern) - if ( - schema.anyOf && - schema.anyOf.length === 2 && - schema.anyOf.some((t) => (t as InspectorFormSchema).type === "string") && - schema.anyOf.some((t) => (t as InspectorFormSchema).type === "null") - ) { - return { ...schema, type: "string", anyOf: undefined, nullable: true }; + if (schema.anyOf && schema.anyOf.length === 2) { + // `JsonSchemaConst` is structurally a subset of `InspectorFormSchema` (it + // just carries no `type`), so reading both as the latter is safe. + const branches = schema.anyOf as InspectorFormSchema[]; + const nullBranch = branches.find((branch) => branch?.type === "null"); + const branch = branches.find((candidate) => candidate?.type !== "null"); + + if (nullBranch && branch) { + // A bare `{ enum: [...] }` branch carries no `type`; JSON Schema allows + // that, and every value such a schema admits here is a string. + const type = branch.type ?? (branch.enum ? "string" : undefined); + if (isRenderableType(type)) { + return { ...schema, ...branch, type, anyOf: undefined, nullable: true }; + } + } } - // Handle anyOf with exactly boolean and null (FastMCP pattern) - if ( - schema.anyOf && - schema.anyOf.length === 2 && - schema.anyOf.some((t) => (t as InspectorFormSchema).type === "boolean") && - schema.anyOf.some((t) => (t as InspectorFormSchema).type === "null") - ) { - return { ...schema, type: "boolean", anyOf: undefined, nullable: true }; - } - - // Handle anyOf with exactly number and null (FastMCP pattern) - if ( - schema.anyOf && - schema.anyOf.length === 2 && - schema.anyOf.some((t) => (t as InspectorFormSchema).type === "number") && - schema.anyOf.some((t) => (t as InspectorFormSchema).type === "null") - ) { - return { ...schema, type: "number", anyOf: undefined, nullable: true }; - } - - // Handle anyOf with exactly integer and null (FastMCP pattern) - if ( - schema.anyOf && - schema.anyOf.length === 2 && - schema.anyOf.some((t) => (t as InspectorFormSchema).type === "integer") && - schema.anyOf.some((t) => (t as InspectorFormSchema).type === "null") - ) { - return { ...schema, type: "integer", anyOf: undefined, nullable: true }; - } - - // Handle anyOf with exactly array and null (FastMCP pattern) - if ( - schema.anyOf && - schema.anyOf.length === 2 && - schema.anyOf.some((t) => (t as InspectorFormSchema).type === "array") && - schema.anyOf.some((t) => (t as InspectorFormSchema).type === "null") - ) { - return { ...schema, type: "array", anyOf: undefined, nullable: true }; - } - - // Handle array type with exactly string and null if ( Array.isArray(schema.type) && schema.type.length === 2 && - schema.type.includes("string") && schema.type.includes("null") ) { - return { ...schema, type: "string", nullable: true }; - } - - // Handle array type with exactly boolean and null - if ( - Array.isArray(schema.type) && - schema.type.length === 2 && - schema.type.includes("boolean") && - schema.type.includes("null") - ) { - return { ...schema, type: "boolean", nullable: true }; - } - - // Handle array type with exactly number and null - if ( - Array.isArray(schema.type) && - schema.type.length === 2 && - schema.type.includes("number") && - schema.type.includes("null") - ) { - return { ...schema, type: "number", nullable: true }; - } - - // Handle array type with exactly integer and null - if ( - Array.isArray(schema.type) && - schema.type.length === 2 && - schema.type.includes("integer") && - schema.type.includes("null") - ) { - return { ...schema, type: "integer", nullable: true }; + const type = schema.type.find((member) => member !== "null"); + if (isRenderableType(type)) { + return { ...schema, type, nullable: true }; + } } return schema; diff --git a/test-servers/configs/nullable-fields-http.json b/test-servers/configs/nullable-fields-http.json new file mode 100644 index 00000000..91c4b647 --- /dev/null +++ b/test-servers/configs/nullable-fields-http.json @@ -0,0 +1,11 @@ +{ + "serverInfo": { + "name": "nullable-fields-showcase", + "version": "1.0.0" + }, + "tools": [{ "preset": "record_shipment" }, { "preset": "get_temp" }], + "transport": { + "type": "streamable-http", + "port": 6602 + } +} diff --git a/test-servers/src/preset-registry.ts b/test-servers/src/preset-registry.ts index 806769b3..d78998ec 100644 --- a/test-servers/src/preset-registry.ts +++ b/test-servers/src/preset-registry.ts @@ -12,6 +12,7 @@ import type { } from "./composable-test-server.js"; import { createEchoTool, + createNullableFieldsTool, createGetEnvTool, createAddTool, createGetSumTool, @@ -91,6 +92,9 @@ function resolveToolPreset( switch (name) { case "echo": return createEchoTool(); + case "record_shipment": + case "nullable_fields": + return createNullableFieldsTool(); case "get-env": case "get_env": return createGetEnvTool(); diff --git a/test-servers/src/test-server-fixtures.ts b/test-servers/src/test-server-fixtures.ts index 5a05b07b..070b5750 100644 --- a/test-servers/src/test-server-fixtures.ts +++ b/test-servers/src/test-server-fixtures.ts @@ -162,6 +162,39 @@ export function createEchoTool(): ToolDefinition { }; } +/** + * Create a "record_shipment" tool whose arguments are all **nullable** — the + * shape Zod's `.nullish()` / `.nullable()` compiles to, where the real type + * sits inside `anyOf: [, { type: "null" }]` rather than at the top + * level. + * + * `direction` is the case from #1928: a nullable *enum*, whose `enum` keyword + * lives on the surviving branch. Before the fix nothing in the tool-call form + * matched such a field, so it fell through to the raw-JSON textarea and + * re-escaped its own value on every keystroke. The other three cover the + * remaining scalar branches so a regression in one is visible next to a + * working sibling. + */ +export function createNullableFieldsTool(): ToolDefinition { + return { + name: "record_shipment", + description: + "Record a shipment. Every argument is optional AND explicitly nullable (Zod .nullish()), so each compiles to an anyOf union with a null branch.", + inputSchema: { + direction: z + .enum(["envio", "recebimento"]) + .nullish() + .describe("Direction of the shipment (nullable enum — #1928)"), + reference: z.string().nullish().describe("Free-text reference"), + quantity: z.number().int().nullish().describe("Number of packages"), + express: z.boolean().nullish().describe("Ship express"), + }, + handler: async (params: Record) => { + return toToolResult(JSON.stringify(params, null, 2)); + }, + }; +} + /** * Create a "get-env" tool matching @modelcontextprotocol/server-everything. * Returns the server process environment as pretty-printed JSON text. From 17a4c78dd3e787293539d619ed3c05cf28c9f251 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 15 Aug 2026 16:59:37 -0400 Subject: [PATCH 11/25] fix: centralize the nullable-union collapse in core and apply it in the TUI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #2015, folded into #1928 rather than left as a follow-up. The TUI's `schemaToForm` has the same defect the web `SchemaForm` had: its local `JsonSchemaProperty` declares only `type?: string` — no `anyOf`, no array-valued `type` — so a `.nullish()` argument has no top-level `type` or `enum` for the dispatcher to read and falls to the `switch` default, losing its select. Milder than the web bug (a plain text field, not a corrupting JSON textarea), but the same class. Fixing it in a second place is the argument for not having the logic in a first place. So the collapse moves to `core/json/nullableUnion.ts` as `normalizeNullableUnion`, generic over a minimal `NullableUnionSchema` shape — the least a caller must expose to be *recognized* as a nullable union — so neither client has to adopt the other's schema type. Both form builders now share one copy and cannot drift on which schemas they render. - `core/json/nullableUnion.ts` — the shared collapse, plus tests at `clients/web/src/test/core/nullableUnion.test.ts` (the web suite is what gates `core/`). - `clients/web`: `schemaUtils.normalizeUnionType` is gone; `SchemaForm` and `resolveRefsInMessage` call the core function directly. - `clients/tui`: `schemaToForm` normalizes each property before dispatch, and `JsonSchemaProperty` grows the `anyOf` / array-`type` fields the encoding uses. Verified by driving the built TUI under a pty against the `nullable-fields-http.json` showcase server. Before, every argument of `record_shipment` rendered as a plain text field; after, `direction` is a select ("No value"), `quantity` an integer, `express` a boolean ("[Not set]"). Signed-off-by: cliffhall --- AGENTS.md | 10 +- README.md | 5 +- clients/tui/__tests__/schemaToForm.test.ts | 113 ++++++++ clients/tui/src/utils/schemaToForm.ts | 23 +- .../groups/SchemaForm/SchemaForm.tsx | 4 +- .../web/src/test/core/nullableUnion.test.ts | 250 ++++++++++++++++++ clients/web/src/utils/schemaUtils.test.ts | 221 ---------------- clients/web/src/utils/schemaUtils.ts | 84 +----- core/json/nullableUnion.ts | 120 +++++++++ 9 files changed, 519 insertions(+), 311 deletions(-) create mode 100644 clients/web/src/test/core/nullableUnion.test.ts create mode 100644 core/json/nullableUnion.ts diff --git a/AGENTS.md b/AGENTS.md index 0baea459..87d51230 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -55,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 diff --git a/README.md b/README.md index 82164a98..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) @@ -234,6 +235,8 @@ The duplicated copies are appended rather than placed beside their twin on purpo 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`. diff --git a/clients/tui/__tests__/schemaToForm.test.ts b/clients/tui/__tests__/schemaToForm.test.ts index 7efca937..93ca1ca9 100644 --- a/clients/tui/__tests__/schemaToForm.test.ts +++ b/clients/tui/__tests__/schemaToForm.test.ts @@ -213,4 +213,117 @@ 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" }, + ], + }); + }); + + 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" }, + ]); + }); + + 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..5201abbb 100644 --- a/clients/tui/src/utils/schemaToForm.ts +++ b/clients/tui/src/utils/schemaToForm.ts @@ -3,10 +3,16 @@ */ import type { FormStructure, FormSection, FormField } from "ink-form"; +import { 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 +21,8 @@ interface JsonSchemaProperty { minimum?: number; maximum?: number; default?: unknown; + /** Present on a nullable union; see {@link normalizeNullableUnion}. */ + anyOf?: readonly unknown[]; } /** @@ -68,9 +76,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, diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx index eadb0564..628f2ee3 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx @@ -12,7 +12,7 @@ import { useState } from "react"; import { ClearButton } from "../../elements/ClearButton/ClearButton"; import { useValueChange } from "../../../hooks/useValueChange"; import type { InspectorFormSchema } from "../../../utils/jsonUtils"; -import { normalizeUnionType } from "../../../utils/schemaUtils"; +import { normalizeNullableUnion } from "@inspector/core/json/nullableUnion.js"; const FieldLabel = Text.withProps({ fw: 500, @@ -224,7 +224,7 @@ export function SchemaForm({ // 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 = normalizeUnionType(rawSchema); + const fieldSchema = normalizeNullableUnion(rawSchema); const isRequired = requiredFields.includes(fieldName); const label = fieldSchema.title ?? fieldName; const description = fieldSchema.description; diff --git a/clients/web/src/test/core/nullableUnion.test.ts b/clients/web/src/test/core/nullableUnion.test.ts new file mode 100644 index 00000000..f56abbdd --- /dev/null +++ b/clients/web/src/test/core/nullableUnion.test.ts @@ -0,0 +1,250 @@ +import { describe, it, expect } from "vitest"; +import { normalizeNullableUnion } from "@inspector/core/json/nullableUnion.js"; + +// The two JSON Schema encodings of "this type, or null". Both form builders — +// the web `SchemaForm` and the TUI's `schemaToForm` — dispatch on a single +// `type` string, so this collapse is what keeps a nullable field renderable +// (#1928 in the web client, #2015 in the TUI). +describe("normalizeNullableUnion", () => { + it("normalizes anyOf string|null", () => { + expect( + normalizeNullableUnion({ + anyOf: [{ type: "string" }, { type: "null" }], + }), + ).toEqual({ + type: "string", + anyOf: undefined, + nullable: true, + }); + }); + + it("normalizes anyOf boolean|null", () => { + expect( + normalizeNullableUnion({ + anyOf: [{ type: "boolean" }, { type: "null" }], + }), + ).toEqual({ + type: "boolean", + anyOf: undefined, + nullable: true, + }); + }); + + it("normalizes anyOf number|null", () => { + expect( + normalizeNullableUnion({ + anyOf: [{ type: "number" }, { type: "null" }], + }), + ).toEqual({ + type: "number", + anyOf: undefined, + nullable: true, + }); + }); + + it("normalizes anyOf integer|null", () => { + expect( + normalizeNullableUnion({ + anyOf: [{ type: "integer" }, { type: "null" }], + }), + ).toEqual({ + type: "integer", + anyOf: undefined, + nullable: true, + }); + }); + + it("normalizes anyOf array|null", () => { + expect( + normalizeNullableUnion({ + anyOf: [{ type: "array" }, { type: "null" }], + }), + ).toEqual({ + type: "array", + anyOf: undefined, + nullable: true, + }); + }); + + it("normalizes type array string|null", () => { + expect(normalizeNullableUnion({ type: ["string", "null"] })).toEqual({ + type: "string", + nullable: true, + }); + }); + + it("normalizes type array boolean|null", () => { + expect(normalizeNullableUnion({ type: ["boolean", "null"] })).toEqual({ + type: "boolean", + nullable: true, + }); + }); + + it("normalizes type array number|null", () => { + expect(normalizeNullableUnion({ type: ["number", "null"] })).toEqual({ + type: "number", + nullable: true, + }); + }); + + it("normalizes type array integer|null", () => { + expect(normalizeNullableUnion({ type: ["integer", "null"] })).toEqual({ + type: "integer", + nullable: true, + }); + }); + + it("returns schema unchanged when no union pattern matches", () => { + const schema = { type: "string" as const }; + expect(normalizeNullableUnion(schema)).toBe(schema); + }); + + it("ignores anyOf with more than two members", () => { + const schema = { + anyOf: [ + { type: "string" as const }, + { type: "null" as const }, + { type: "number" as const }, + ], + }; + expect(normalizeNullableUnion(schema)).toBe(schema); + }); + + it("ignores a two-member anyOf with no null branch", () => { + const schema = { + anyOf: [{ type: "string" as const }, { type: "number" as const }], + }; + expect(normalizeNullableUnion(schema)).toBe(schema); + }); + + it("ignores an anyOf whose non-null branch has no renderable type (#1928)", () => { + const schema = { + anyOf: [{ const: "only" }, { type: "null" as const }], + }; + expect(normalizeNullableUnion(schema)).toBe(schema); + }); + + // #1928: the enum lives on the surviving branch, and hoisting it is what makes + // the field render as a Select instead of the raw-JSON fallback. + it("hoists enum out of an anyOf string-enum|null branch", () => { + expect( + normalizeNullableUnion({ + description: "Direction", + anyOf: [ + { type: "string", enum: ["envio", "recebimento"] }, + { type: "null" }, + ], + }), + ).toEqual({ + type: "string", + description: "Direction", + enum: ["envio", "recebimento"], + anyOf: undefined, + nullable: true, + }); + }); + + it("infers string for a typeless enum branch", () => { + expect( + normalizeNullableUnion({ + anyOf: [{ enum: ["a", "b"] }, { type: "null" }], + }), + ).toEqual({ + type: "string", + enum: ["a", "b"], + anyOf: undefined, + nullable: true, + }); + }); + + it("hoists items out of an anyOf array|null branch", () => { + expect( + normalizeNullableUnion({ + anyOf: [ + { type: "array", items: { type: "string", enum: ["a", "b"] } }, + { type: "null" }, + ], + }), + ).toEqual({ + type: "array", + items: { type: "string", enum: ["a", "b"] }, + anyOf: undefined, + nullable: true, + }); + }); + + it("hoists properties out of an anyOf object|null branch", () => { + expect( + normalizeNullableUnion({ + anyOf: [ + { type: "object", properties: { a: { type: "string" } } }, + { type: "null" }, + ], + }), + ).toEqual({ + type: "object", + properties: { a: { type: "string" } }, + anyOf: undefined, + nullable: true, + }); + }); + + it("normalizes anyOf object|null regardless of branch order", () => { + expect( + normalizeNullableUnion({ + anyOf: [{ type: "null" }, { type: "object" }], + }), + ).toEqual({ type: "object", anyOf: undefined, nullable: true }); + }); + + it("normalizes type array object|null", () => { + expect( + normalizeNullableUnion({ + type: ["object", "null"], + properties: { a: { type: "string" } }, + }), + ).toEqual({ + type: "object", + properties: { a: { type: "string" } }, + nullable: true, + }); + }); + + it("normalizes type array array|null", () => { + expect(normalizeNullableUnion({ type: ["array", "null"] })).toEqual({ + type: "array", + nullable: true, + }); + }); + + it("keeps a type array of two non-null members unchanged", () => { + const schema = { type: ["string", "number"] as const }; + expect( + normalizeNullableUnion({ ...schema, type: [...schema.type] }), + ).toEqual({ + type: ["string", "number"], + }); + }); + + // A server can put anything in `anyOf`; a non-object member must not be read + // as a branch (nor throw), it simply means no nullable union was recognized. + it("ignores an anyOf holding a non-object member", () => { + const schema = { anyOf: ["string", { type: "null" as const }] }; + expect(normalizeNullableUnion(schema)).toBe(schema); + }); + + it("ignores an anyOf holding an array member", () => { + const schema = { anyOf: [["string"], { type: "null" as const }] }; + expect(normalizeNullableUnion(schema)).toBe(schema); + }); + + it("ignores a type array whose non-null member has no widget", () => { + const schema = { type: ["null", "null"] }; + expect(normalizeNullableUnion(schema)).toBe(schema); + }); + + it("ignores a schema with no union keywords at all", () => { + const schema = { type: "object" as const, properties: {} }; + expect(normalizeNullableUnion(schema)).toBe(schema); + }); +}); diff --git a/clients/web/src/utils/schemaUtils.test.ts b/clients/web/src/utils/schemaUtils.test.ts index 59623fdb..7ff38c88 100644 --- a/clients/web/src/utils/schemaUtils.test.ts +++ b/clients/web/src/utils/schemaUtils.test.ts @@ -8,7 +8,6 @@ import { generateDefaultValue, isPropertyRequired, resolveRef, - normalizeUnionType, formatFieldLabel, resolveRefsInMessage, } from "./schemaUtils"; @@ -243,226 +242,6 @@ describe("resolveRef", () => { }); }); -describe("normalizeUnionType", () => { - it("normalizes anyOf string|null", () => { - expect( - normalizeUnionType({ - anyOf: [{ type: "string" }, { type: "null" }], - }), - ).toEqual({ - type: "string", - anyOf: undefined, - nullable: true, - }); - }); - - it("normalizes anyOf boolean|null", () => { - expect( - normalizeUnionType({ - anyOf: [{ type: "boolean" }, { type: "null" }], - }), - ).toEqual({ - type: "boolean", - anyOf: undefined, - nullable: true, - }); - }); - - it("normalizes anyOf number|null", () => { - expect( - normalizeUnionType({ - anyOf: [{ type: "number" }, { type: "null" }], - }), - ).toEqual({ - type: "number", - anyOf: undefined, - nullable: true, - }); - }); - - it("normalizes anyOf integer|null", () => { - expect( - normalizeUnionType({ - anyOf: [{ type: "integer" }, { type: "null" }], - }), - ).toEqual({ - type: "integer", - anyOf: undefined, - nullable: true, - }); - }); - - it("normalizes anyOf array|null", () => { - expect( - normalizeUnionType({ - anyOf: [{ type: "array" }, { type: "null" }], - }), - ).toEqual({ - type: "array", - anyOf: undefined, - nullable: true, - }); - }); - - it("normalizes type array string|null", () => { - expect(normalizeUnionType({ type: ["string", "null"] })).toEqual({ - type: "string", - nullable: true, - }); - }); - - it("normalizes type array boolean|null", () => { - expect(normalizeUnionType({ type: ["boolean", "null"] })).toEqual({ - type: "boolean", - nullable: true, - }); - }); - - it("normalizes type array number|null", () => { - expect(normalizeUnionType({ type: ["number", "null"] })).toEqual({ - type: "number", - nullable: true, - }); - }); - - it("normalizes type array integer|null", () => { - expect(normalizeUnionType({ type: ["integer", "null"] })).toEqual({ - type: "integer", - nullable: true, - }); - }); - - it("returns schema unchanged when no union pattern matches", () => { - const schema = { type: "string" as const }; - expect(normalizeUnionType(schema)).toBe(schema); - }); - - it("ignores anyOf with more than two members", () => { - const schema = { - anyOf: [ - { type: "string" as const }, - { type: "null" as const }, - { type: "number" as const }, - ], - }; - expect(normalizeUnionType(schema)).toBe(schema); - }); - - it("ignores a two-member anyOf with no null branch", () => { - const schema = { - anyOf: [{ type: "string" as const }, { type: "number" as const }], - }; - expect(normalizeUnionType(schema)).toBe(schema); - }); - - it("ignores an anyOf whose non-null branch has no renderable type (#1928)", () => { - const schema = { - anyOf: [{ const: "only" }, { type: "null" as const }], - }; - expect(normalizeUnionType(schema)).toBe(schema); - }); - - // #1928: the enum lives on the surviving branch, and hoisting it is what makes - // the field render as a Select instead of the raw-JSON fallback. - it("hoists enum out of an anyOf string-enum|null branch", () => { - expect( - normalizeUnionType({ - description: "Direction", - anyOf: [ - { type: "string", enum: ["envio", "recebimento"] }, - { type: "null" }, - ], - }), - ).toEqual({ - type: "string", - description: "Direction", - enum: ["envio", "recebimento"], - anyOf: undefined, - nullable: true, - }); - }); - - it("infers string for a typeless enum branch", () => { - expect( - normalizeUnionType({ - anyOf: [{ enum: ["a", "b"] }, { type: "null" }], - }), - ).toEqual({ - type: "string", - enum: ["a", "b"], - anyOf: undefined, - nullable: true, - }); - }); - - it("hoists items out of an anyOf array|null branch", () => { - expect( - normalizeUnionType({ - anyOf: [ - { type: "array", items: { type: "string", enum: ["a", "b"] } }, - { type: "null" }, - ], - }), - ).toEqual({ - type: "array", - items: { type: "string", enum: ["a", "b"] }, - anyOf: undefined, - nullable: true, - }); - }); - - it("hoists properties out of an anyOf object|null branch", () => { - expect( - normalizeUnionType({ - anyOf: [ - { type: "object", properties: { a: { type: "string" } } }, - { type: "null" }, - ], - }), - ).toEqual({ - type: "object", - properties: { a: { type: "string" } }, - anyOf: undefined, - nullable: true, - }); - }); - - it("normalizes anyOf object|null regardless of branch order", () => { - expect( - normalizeUnionType({ - anyOf: [{ type: "null" }, { type: "object" }], - }), - ).toEqual({ type: "object", anyOf: undefined, nullable: true }); - }); - - it("normalizes type array object|null", () => { - expect( - normalizeUnionType({ - type: ["object", "null"], - properties: { a: { type: "string" } }, - }), - ).toEqual({ - type: "object", - properties: { a: { type: "string" } }, - nullable: true, - }); - }); - - it("normalizes type array array|null", () => { - expect(normalizeUnionType({ type: ["array", "null"] })).toEqual({ - type: "array", - nullable: true, - }); - }); - - it("keeps a type array of two non-null members unchanged", () => { - const schema = { type: ["string", "number"] as const }; - expect(normalizeUnionType({ ...schema, type: [...schema.type] })).toEqual({ - type: ["string", "number"], - }); - }); -}); - describe("formatFieldLabel", () => { it("inserts spaces before capitals and capitalizes first letter", () => { expect(formatFieldLabel("firstName")).toBe("First Name"); diff --git a/clients/web/src/utils/schemaUtils.ts b/clients/web/src/utils/schemaUtils.ts index 2624fba1..b03582c0 100644 --- a/clients/web/src/utils/schemaUtils.ts +++ b/clients/web/src/utils/schemaUtils.ts @@ -3,6 +3,7 @@ import Ajv from "ajv"; import type { ValidateFunction } from "ajv"; import type { Tool, JSONRPCMessage } from "@modelcontextprotocol/client"; import { isJSONRPCRequest } from "@modelcontextprotocol/client"; +import { normalizeNullableUnion } from "@inspector/core/json/nullableUnion.js"; const ajv = new Ajv(); @@ -200,87 +201,6 @@ export function resolveRef( return schema; } -/** - * The `type` values {@link SchemaForm} dispatches on. `"null"` is deliberately - * absent: it is the branch a nullable union is being flattened *away* from, and - * there is no widget for a field whose only permitted value is `null`. - */ -const RENDERABLE_TYPES = [ - "string", - "number", - "integer", - "boolean", - "array", - "object", -] as const; - -type RenderableType = (typeof RENDERABLE_TYPES)[number]; - -function isRenderableType(type: unknown): type is RenderableType { - return RENDERABLE_TYPES.includes(type as RenderableType); -} - -/** - * Normalizes a nullable union (`string|null` from FastMCP, or anything Zod's - * `.nullish()` / `.nullable()` compiles to) into the plain type the form - * renderer dispatches on. - * - * Two encodings mean the same thing and are both flattened to - * `{ type: , nullable: true }`: - * - * - `anyOf: [, { type: "null" }]` — the branch's *own* keywords are - * hoisted onto the result, because that is where the detail the renderer - * needs lives. A nullable enum compiles to - * `anyOf: [{ type: "string", enum: [...] }, { type: "null" }]`, so hoisting - * `enum` is what makes it a `Select` instead of falling through to the raw - * JSON fallback (#1928). The branch also wins over the wrapper on any shared - * key, matching v1.x's behavior. - * - `type: [, "null"]` — the keywords already sit at the top level, so only - * `type` collapses. - * - * Anything else — a union of two real types, a three-member `anyOf`, a branch - * whose type has no widget — is returned untouched and renders through the - * JSON fallback, which is the honest representation of a shape the form cannot - * model. - * - * @param schema The JSON schema to normalize - * @returns A normalized schema, or the original schema when no nullable-union - * pattern matches - */ -export function normalizeUnionType( - schema: InspectorFormSchema, -): InspectorFormSchema { - if (schema.anyOf && schema.anyOf.length === 2) { - // `JsonSchemaConst` is structurally a subset of `InspectorFormSchema` (it - // just carries no `type`), so reading both as the latter is safe. - const branches = schema.anyOf as InspectorFormSchema[]; - const nullBranch = branches.find((branch) => branch?.type === "null"); - const branch = branches.find((candidate) => candidate?.type !== "null"); - - if (nullBranch && branch) { - // A bare `{ enum: [...] }` branch carries no `type`; JSON Schema allows - // that, and every value such a schema admits here is a string. - const type = branch.type ?? (branch.enum ? "string" : undefined); - if (isRenderableType(type)) { - return { ...schema, ...branch, type, anyOf: undefined, nullable: true }; - } - } - } - - if ( - Array.isArray(schema.type) && - schema.type.length === 2 && - schema.type.includes("null") - ) { - const type = schema.type.find((member) => member !== "null"); - if (isRenderableType(type)) { - return { ...schema, type, nullable: true }; - } - } - - return schema; -} - /** * Formats a field key into a human-readable label * @param key The field key to format @@ -319,7 +239,7 @@ export function resolveRefsInMessage(message: JSONRPCMessage): JSONRPCMessage { Object.entries(requestedSchema.properties).map( ([key, propSchema]) => { const resolved = resolveRef(propSchema, requestedSchema); - const normalized = normalizeUnionType(resolved); + const normalized = normalizeNullableUnion(resolved); return [key, normalized]; }, ), diff --git a/core/json/nullableUnion.ts b/core/json/nullableUnion.ts new file mode 100644 index 00000000..4e81b791 --- /dev/null +++ b/core/json/nullableUnion.ts @@ -0,0 +1,120 @@ +/** + * Flattening of the two JSON Schema encodings of "this type, **or** null" into + * the plain type a form renderer can dispatch on. + * + * Shared because both form builders — the web client's `SchemaForm` and the + * TUI's `schemaToForm` — dispatch on a *single* `type` string, and so both miss + * a nullable field entirely without this step (#1928, #2015). Keeping one copy + * is what stops the two clients from disagreeing about which schemas they can + * render; the alternative was two implementations of the same subtle predicate. + */ + +/** + * JSON Schema type names a form renderer can build a widget for. + * + * `"null"` is deliberately absent: it is the branch a nullable union is being + * flattened *away* from, and there is no widget for a field whose only + * permitted value is `null`. + */ +const RENDERABLE_TYPES = [ + "string", + "number", + "integer", + "boolean", + "array", + "object", +] as const; + +type RenderableType = (typeof RENDERABLE_TYPES)[number]; + +function isRenderableType(type: unknown): type is RenderableType { + return RENDERABLE_TYPES.includes(type as RenderableType); +} + +/** + * The keywords {@link normalizeNullableUnion} reads. Both clients' own schema + * types are structurally assignable to this, so neither has to adopt the + * other's — the shape is deliberately the minimum needed to *recognize* a + * nullable union, not a full JSON Schema model. + */ +export interface NullableUnionSchema { + type?: string | string[]; + enum?: unknown[]; + anyOf?: readonly unknown[]; +} + +/** Narrow an `anyOf` member to a readable object, or `null` if it isn't one. */ +function toBranch(value: unknown): Record | null { + if (typeof value !== "object" || value === null || Array.isArray(value)) { + return null; + } + return value as Record; +} + +/** + * Collapse a nullable union — what Zod's `.nullish()` / `.nullable()` and + * FastMCP's optional arguments emit — into `{ type: , nullable: true }`. + * + * Two encodings mean the same thing and are both handled: + * + * - `anyOf: [, { type: "null" }]` — the branch's *own* keywords are + * hoisted onto the result, because that is where the detail a renderer needs + * lives. A nullable enum compiles to + * `anyOf: [{ type: "string", enum: [...] }, { type: "null" }]`, so hoisting + * `enum` is what makes it a dropdown rather than a raw-JSON fallback (#1928). + * The branch also wins over the wrapper on any shared key, matching v1.x. + * - `type: [, "null"]` — the keywords already sit at the top level, so only + * `type` collapses. + * + * Anything else — a union of two real types, a three-member `anyOf`, a branch + * whose type has no widget — is returned **by identity**, so a caller can use + * `===` to tell that nothing was recognized. + * + * @param schema The schema to normalize + * @returns A flattened copy, or `schema` itself when no nullable-union pattern + * matches + */ +export function normalizeNullableUnion( + schema: T, +): T { + if (schema.anyOf?.length === 2) { + const branches = schema.anyOf.map(toBranch); + const nullBranch = branches.find((entry) => entry?.type === "null"); + const branch = branches.find( + (entry) => entry !== null && entry.type !== "null", + ); + + if (nullBranch && branch) { + // A bare `{ enum: [...] }` branch carries no `type`; JSON Schema allows + // that, and every value such a schema admits here is a string. + const type = branch.type ?? (branch.enum ? "string" : undefined); + if (isRenderableType(type)) { + // The result is `schema` with `type` narrowed to a value it already + // admitted, the branch's keywords merged in, and `nullable` added. TS + // cannot express "a spread of T and a subset of T is still a T", and a + // non-generic return would force every caller to cast instead. + return { + ...schema, + ...branch, + type, + anyOf: undefined, + nullable: true, + } as T; + } + } + } + + if ( + Array.isArray(schema.type) && + schema.type.length === 2 && + schema.type.includes("null") + ) { + const type = schema.type.find((member) => member !== "null"); + if (isRenderableType(type)) { + // Same reasoning as above. + return { ...schema, type, nullable: true } as T; + } + } + + return schema; +} From e4e16bb3e6783a7ca08e9072e4c899cd33b6544c Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 15 Aug 2026 17:24:46 -0400 Subject: [PATCH 12/25] fix: address Copilot review on the nullable-union collapse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings, all valid: 1. Returning `T` was unsound. The collapse genuinely changes two fields — `type` stops being an array, `anyOf` becomes `undefined` — so a caller whose `T` typed either that way could keep treating it so afterwards. The result is now `T | NormalizedNullableUnion`, which states exactly that: `Omit` plus a `RenderableType` `type`, a cleared `anyOf`, and `nullable`. The one assertion left is isolated in a `collapsed()` helper, needed only because `Omit` stays deferred while `T` is open. 2. A typeless `enum` does not imply strings. JSON Schema allows `enum: [1, 2]`, and inferring `"string"` for it would hand numbers to a `Select` declared `string[]`, and make the TUI submit `"1"` for `1`. Now only an all-string enum earns the inference; anything else stays on the JSON fallback. An explicit `type` is still authoritative. 3. `hasMissingRequiredFields` counted every null as missing, so clearing a *required* nullable enum — newly possible via the clear button — would disable submit on a value the schema permits. `required` constrains presence, not content, so null now counts as present exactly when the field's schema admits it. 4. `collectSchemaDefaults` read the raw union, so a default nested inside a nullable object's branch was displayed by the form but never seeded into the submitted values. It normalizes each property first, like the renderer does. Signed-off-by: cliffhall --- .../web/src/test/core/nullableUnion.test.ts | 33 +++++++ clients/web/src/utils/jsonUtils.test.ts | 76 +++++++++++++++ clients/web/src/utils/jsonUtils.ts | 25 ++++- core/json/nullableUnion.ts | 93 +++++++++++++++---- 4 files changed, 207 insertions(+), 20 deletions(-) diff --git a/clients/web/src/test/core/nullableUnion.test.ts b/clients/web/src/test/core/nullableUnion.test.ts index f56abbdd..915e7e8e 100644 --- a/clients/web/src/test/core/nullableUnion.test.ts +++ b/clients/web/src/test/core/nullableUnion.test.ts @@ -144,6 +144,39 @@ describe("normalizeNullableUnion", () => { }); }); + // JSON Schema's `enum` is untyped, so a typeless branch only implies strings + // when every member is one. Guessing otherwise would hand a number to a + // renderer that declared the option list `string[]`. + it("does not infer string for a typeless non-string enum branch", () => { + const schema = { anyOf: [{ enum: [1, 2] }, { type: "null" as const }] }; + expect(normalizeNullableUnion(schema)).toBe(schema); + }); + + it("does not infer string for a typeless mixed enum branch", () => { + const schema = { anyOf: [{ enum: ["a", 2] }, { type: "null" as const }] }; + expect(normalizeNullableUnion(schema)).toBe(schema); + }); + + it("does not infer string for a typeless empty enum branch", () => { + const schema = { anyOf: [{ enum: [] }, { type: "null" as const }] }; + expect(normalizeNullableUnion(schema)).toBe(schema); + }); + + // An explicit `type` is authoritative — the enum members are the server's + // problem at that point, not an inference this function is making. + it("still collapses a non-string enum branch that declares its type", () => { + expect( + normalizeNullableUnion({ + anyOf: [{ type: "number", enum: [1, 2] }, { type: "null" }], + }), + ).toEqual({ + type: "number", + enum: [1, 2], + anyOf: undefined, + nullable: true, + }); + }); + it("infers string for a typeless enum branch", () => { expect( normalizeNullableUnion({ diff --git a/clients/web/src/utils/jsonUtils.test.ts b/clients/web/src/utils/jsonUtils.test.ts index 57a61132..4a69409c 100644 --- a/clients/web/src/utils/jsonUtils.test.ts +++ b/clients/web/src/utils/jsonUtils.test.ts @@ -221,6 +221,42 @@ describe("collectSchemaDefaults", () => { it("returns an empty object when the schema has no properties", () => { expect(collectSchemaDefaults({ type: "object" })).toEqual({}); }); + + // The form normalizes a nullable union before rendering, so this has to as + // well: a nested object's `properties` live on the union's surviving branch, + // and a default only visible there would be displayed but never submitted. + it("collects defaults hoisted out of a nullable object union", () => { + const schema: InspectorFormSchema = { + type: "object", + properties: { + profile: { + anyOf: [ + { + type: "object", + properties: { nick: { type: "string", default: "ada" } }, + }, + { type: "null" }, + ], + }, + }, + }; + expect(collectSchemaDefaults(schema)).toEqual({ + profile: { nick: "ada" }, + }); + }); + + it("keeps a default declared on the union wrapper itself", () => { + const schema: InspectorFormSchema = { + type: "object", + properties: { + direction: { + default: "envio", + anyOf: [{ type: "string", enum: ["envio"] }, { type: "null" }], + }, + }, + }; + expect(collectSchemaDefaults(schema)).toEqual({ direction: "envio" }); + }); }); describe("hasMissingRequiredFields", () => { @@ -247,4 +283,44 @@ describe("hasMissingRequiredFields", () => { false, ); }); + + // JSON Schema `required` constrains presence, not content — so an explicit + // `null` satisfies a required field whose schema admits null. Since #1928 the + // user can produce exactly that by clearing a nullable enum, and treating it + // as missing would disable submit on a value the schema calls valid. + it("accepts an explicit null for a required field that admits null", () => { + const nullable: InspectorFormSchema = { + type: "object", + properties: { + direction: { + anyOf: [{ type: "string", enum: ["envio"] }, { type: "null" }], + }, + }, + required: ["direction"], + }; + expect(hasMissingRequiredFields(nullable, { direction: null })).toBe(false); + expect(hasMissingRequiredFields(nullable, {})).toBe(true); + }); + + it("accepts an explicit null for a required type: [T, null] field", () => { + const nullable: InspectorFormSchema = { + type: "object", + properties: { note: { type: ["string", "null"] } }, + required: ["note"], + }; + expect(hasMissingRequiredFields(nullable, { note: null })).toBe(false); + }); + + it("still rejects null for a required field whose schema is not nullable", () => { + expect(hasMissingRequiredFields(schema, { name: null })).toBe(true); + }); + + it("rejects null for a required field the schema does not describe", () => { + const undescribed: InspectorFormSchema = { + type: "object", + properties: {}, + required: ["ghost"], + }; + expect(hasMissingRequiredFields(undescribed, { ghost: null })).toBe(true); + }); }); diff --git a/clients/web/src/utils/jsonUtils.ts b/clients/web/src/utils/jsonUtils.ts index 98dd905a..2cdb2331 100644 --- a/clients/web/src/utils/jsonUtils.ts +++ b/clients/web/src/utils/jsonUtils.ts @@ -1,3 +1,5 @@ +import { normalizeNullableUnion } from "@inspector/core/json/nullableUnion.js"; + export type JsonValue = | string | number @@ -116,7 +118,12 @@ export function collectSchemaDefaults( ): Record { const properties = schema.properties ?? {}; const result: Record = {}; - for (const [fieldName, fieldSchema] of Object.entries(properties)) { + for (const [fieldName, rawSchema] of Object.entries(properties)) { + // Collapse a nullable union first, for the same reason `SchemaForm` does: + // a nested object's `properties` live on the union's surviving branch, so + // without this the form would *display* a hoisted default that never + // reached the seeded values — the field would submit empty (#1928). + const fieldSchema = normalizeNullableUnion(rawSchema); if (fieldSchema.default !== undefined) { result[fieldName] = fieldSchema.default; } else if (fieldSchema.type === "object" && fieldSchema.properties) { @@ -133,15 +140,29 @@ export function collectSchemaDefaults( * Whether any of the schema's required top-level fields is missing a value in * `values` (absent, null, or empty string). Used to gate a form's submit * action until required fields are supplied. + * + * `null` counts as missing only for a field that does not admit it. JSON + * Schema's `required` constrains *presence*, not content, so a required field + * whose schema is a nullable union is satisfied by an explicit `null` — and + * since #1928 the user can produce exactly that by clearing a nullable enum. + * Treating it as missing would leave the form's submit permanently disabled on + * a value the schema calls valid. */ export function hasMissingRequiredFields( schema: InspectorFormSchema, values: Record, ): boolean { const required = schema.required ?? []; + const properties = schema.properties ?? {}; return required.some((field) => { const value = values[field]; - return value === undefined || value === null || value === ""; + if (value === null) { + const fieldSchema = properties[field]; + return fieldSchema === undefined + ? true + : normalizeNullableUnion(fieldSchema).nullable !== true; + } + return value === undefined || value === ""; }); } diff --git a/core/json/nullableUnion.ts b/core/json/nullableUnion.ts index 4e81b791..2adfdd51 100644 --- a/core/json/nullableUnion.ts +++ b/core/json/nullableUnion.ts @@ -25,7 +25,8 @@ const RENDERABLE_TYPES = [ "object", ] as const; -type RenderableType = (typeof RENDERABLE_TYPES)[number]; +/** A `type` name the collapse is willing to produce. */ +export type RenderableType = (typeof RENDERABLE_TYPES)[number]; function isRenderableType(type: unknown): type is RenderableType { return RENDERABLE_TYPES.includes(type as RenderableType); @@ -51,6 +52,71 @@ function toBranch(value: unknown): Record | null { return value as Record; } +/** + * Whether a branch's `enum` is one a *typeless* branch can be read as a string + * enum. + * + * JSON Schema's `enum` is untyped — `[1, 2]`, `[true, false]`, and `[null]` are + * all legal — so a bare `{ enum: [...] }` does **not** imply strings. Guessing + * `"string"` for a numeric enum would hand non-strings to a renderer that has + * declared them `string[]`: the web `Select` would receive numbers, and the TUI + * would `String(...)` them and submit `"1"` where the server expects `1`. So + * only an all-string enum earns the inference; anything else stays on the + * fallback path, which renders the value honestly as JSON. + */ +function isStringEnum(value: unknown): boolean { + return ( + Array.isArray(value) && + value.length > 0 && + value.every((member) => typeof member === "string") + ); +} + +/** + * The result of a collapse that actually matched. Spelled out rather than + * reusing `T` because the collapse genuinely changes two of `T`'s fields: + * `type` becomes a single renderable name (never an array, never `"null"`), + * and `anyOf` is cleared. Returning `T` for these would let a caller whose `T` + * types `type` as an array go on treating it as one after it has become a + * string, or dereference an `anyOf` that is now `undefined`. + * + * Note this cannot make the *hoist* sound: an `anyOf` branch is `unknown`, so + * any keyword lifted off it is whatever the server sent, however `T` declares + * it. {@link isStringEnum} validates the one keyword the renderers dereference + * as a typed array (`enum`); the rest reach widgets that read them defensively. + */ +export type NormalizedNullableUnion = Omit< + T, + "type" | "anyOf" +> & { + type?: RenderableType; + anyOf?: undefined; + nullable?: boolean; +}; + +/** + * Build the collapsed schema. + * + * The assertion is needed because the spread merges an unresolved generic `T` + * with an `unknown` branch, and `NormalizedNullableUnion`'s `Omit` stays + * deferred while `T` is open — so TS cannot verify the two line up even though + * every field is set right here. Isolated in this one function so the exported + * signature carries the contract and nothing else has to assert it. + */ +function collapsed( + schema: T, + branch: Record | undefined, + type: RenderableType, +): NormalizedNullableUnion { + return { + ...schema, + ...branch, + type, + anyOf: undefined, + nullable: true, + } as NormalizedNullableUnion; +} + /** * Collapse a nullable union — what Zod's `.nullish()` / `.nullable()` and * FastMCP's optional arguments emit — into `{ type: , nullable: true }`. @@ -76,7 +142,7 @@ function toBranch(value: unknown): Record | null { */ export function normalizeNullableUnion( schema: T, -): T { +): T | NormalizedNullableUnion { if (schema.anyOf?.length === 2) { const branches = schema.anyOf.map(toBranch); const nullBranch = branches.find((entry) => entry?.type === "null"); @@ -85,21 +151,13 @@ export function normalizeNullableUnion( ); if (nullBranch && branch) { - // A bare `{ enum: [...] }` branch carries no `type`; JSON Schema allows - // that, and every value such a schema admits here is a string. - const type = branch.type ?? (branch.enum ? "string" : undefined); + // A branch may carry an `enum` and no `type`; JSON Schema allows that, and + // an all-string enum is unambiguously a string field. See isStringEnum for + // why a non-string enum deliberately does not get the same treatment. + const type = + branch.type ?? (isStringEnum(branch.enum) ? "string" : undefined); if (isRenderableType(type)) { - // The result is `schema` with `type` narrowed to a value it already - // admitted, the branch's keywords merged in, and `nullable` added. TS - // cannot express "a spread of T and a subset of T is still a T", and a - // non-generic return would force every caller to cast instead. - return { - ...schema, - ...branch, - type, - anyOf: undefined, - nullable: true, - } as T; + return collapsed(schema, branch, type); } } } @@ -111,8 +169,7 @@ export function normalizeNullableUnion( ) { const type = schema.type.find((member) => member !== "null"); if (isRenderableType(type)) { - // Same reasoning as above. - return { ...schema, type, nullable: true } as T; + return collapsed(schema, undefined, type); } } From 2b2e1575111ec8911ccec63a3c916a36ee5b15b5 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 15 Aug 2026 17:39:43 -0400 Subject: [PATCH 13/25] fix: gate the connect-time logging/setLevel on the negotiated era (#1990) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `logging/setLevel` is a legacy-era method, but connect() called it on any server advertising the `logging` capability, era regardless. The modern wire era rejects it outright, and because the call sits in the post-handshake sequence that rejection failed the *connection* — so the CLI (the one caller passing `initialLoggingLevel`, hardcoded to "debug") could not run any method at all against a modern server that advertised logging. Gate it on `protocolEra !== "modern"`. Modern has no session-scoped level to set: the equivalent is the per-request `io.modelcontextprotocol/logLevel` `_meta` opt-in, already driven by `modernLogLevel` from the server settings. So modern skips the call rather than translating it. Legacy is unchanged. The guard lives next to the capability check in inspectorClient.ts rather than in the CLI, so any future caller passing `initialLoggingLevel` is covered too. Signed-off-by: cliffhall --- ...nspectorClient-initial-logging-era.test.ts | 127 ++++++++++++++++++ core/mcp/inspectorClient.ts | 19 ++- 2 files changed, 144 insertions(+), 2 deletions(-) create mode 100644 clients/web/src/test/core/mcp/inspectorClient-initial-logging-era.test.ts diff --git a/clients/web/src/test/core/mcp/inspectorClient-initial-logging-era.test.ts b/clients/web/src/test/core/mcp/inspectorClient-initial-logging-era.test.ts new file mode 100644 index 00000000..1f5fa165 --- /dev/null +++ b/clients/web/src/test/core/mcp/inspectorClient-initial-logging-era.test.ts @@ -0,0 +1,127 @@ +import { describe, it, expect } from "vitest"; +import type { JSONRPCMessage, Transport } from "@modelcontextprotocol/client"; +import { InspectorClient } from "@inspector/core/mcp/inspectorClient.js"; +import { + MODERN_PROTOCOL_VERSION, + eraToVersionNegotiation, +} from "@inspector/core/mcp/types.js"; + +/** + * Regression coverage for #1990: the connect-time `initialLoggingLevel` must be + * gated on the negotiated era, not on the server capability alone. + * + * `logging/setLevel` is a legacy-era method, and the modern wire era rejects it + * outright ("Method 'logging/setLevel' is not supported by the negotiated + * protocol version"). Because the call sat in `connect()`'s post-handshake + * sequence, that rejection failed the *connection* — so the CLI (the one caller + * that passes `initialLoggingLevel`, hardcoded to "debug" in + * `clients/cli/src/cli.ts`) could not run *any* method against a modern server + * that advertised `logging`, logging-related or not. + * + * The two cases below are the era split: legacy still sends the request with the + * configured level, modern connects cleanly and never puts the method on the + * wire. The modern half asserts on the sent methods rather than on connect + * succeeding alone — a server that happens to answer the method would hide the + * regression behind a green connect. + * + * A fake transport carries both. The modern leg needs no `initialize` at all: + * the SDK negotiates with a `server/discover` probe and adopts that result's + * capabilities directly, which is also what lets one small class serve both eras + * by answering whichever handshake arrives. + */ +class CapabilityAdvertisingTransport implements Transport { + onmessage?: (message: JSONRPCMessage) => void; + onclose?: () => void; + onerror?: (error: Error) => void; + + /** Every method this client put on the wire, in order. */ + readonly sentMethods: string[] = []; + + /** The params of the `logging/setLevel` request, if one was sent. */ + loggingSetLevelParams?: Record; + + async start(): Promise {} + + async close(): Promise { + this.onclose?.(); + } + + async send(message: JSONRPCMessage): Promise { + if (!("method" in message)) return; + this.sentMethods.push(message.method); + + // The modern era's handshake: one probe, answered with the capabilities the + // client then treats as the server's. + if (message.method === "server/discover" && "id" in message) { + this.reply(message.id, { + supportedVersions: [MODERN_PROTOCOL_VERSION], + capabilities: { logging: {} }, + }); + return; + } + + // The legacy era's handshake. + if (message.method === "initialize" && "id" in message) { + const params = message.params as { protocolVersion: string }; + this.reply(message.id, { + protocolVersion: params.protocolVersion, + capabilities: { logging: {} }, + serverInfo: { name: "logging-server", version: "1.0.0" }, + }); + return; + } + + if (message.method === "logging/setLevel" && "id" in message) { + this.loggingSetLevelParams = message.params as Record; + this.reply(message.id, {}); + return; + } + } + + private reply(id: string | number, result: Record): void { + // The SDK resolves the request from its own microtask; delivering + // synchronously from inside `send()` is the earliest any server could. + this.onmessage?.({ jsonrpc: "2.0", id, result }); + } +} + +async function connectWith( + era: "legacy" | "modern", +): Promise { + const transport = new CapabilityAdvertisingTransport(); + const client = new InspectorClient( + { type: "streamable-http", url: "https://mcp.example/mcp" }, + { + environment: { transport: () => ({ transport }) }, + versionNegotiation: eraToVersionNegotiation(era), + initialLoggingLevel: "debug", + }, + ); + + await client.connect(); + expect(client.getProtocolEra()).toBe(era); + await client.disconnect(); + + return transport; +} + +describe("InspectorClient connect() initial logging level, by era (#1990)", () => { + it("sends logging/setLevel on a legacy connection", async () => { + const transport = await connectWith("legacy"); + + expect(transport.sentMethods).toContain("logging/setLevel"); + expect(transport.loggingSetLevelParams).toMatchObject({ level: "debug" }); + }); + + it("never sends logging/setLevel on a modern connection", async () => { + const transport = await connectWith("modern"); + + // Modern has no session-scoped level: the equivalent is the per-request + // `io.modelcontextprotocol/logLevel` `_meta` opt-in, driven by the + // `modernLogLevel` server setting rather than by `initialLoggingLevel`. + expect(transport.sentMethods).not.toContain("logging/setLevel"); + // The server did advertise logging, so the capability check alone would + // have let the call through — the era gate is what stops it. + expect(transport.sentMethods).toContain("server/discover"); + }); +}); diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index b0b809f3..5d8aa86a 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -1936,8 +1936,23 @@ export class InspectorClient extends InspectorClientEventTarget { // capabilities and wipe tools/prompts/resources to empty on every connect. await this.fetchServerInfo(); - // Set initial logging level if configured and server supports it - if (this.initialLoggingLevel && this.capabilities?.logging) { + // Set initial logging level if configured and server supports it. + // + // Era-gated (#1990): `logging/setLevel` is a legacy-era method, and the + // modern wire era rejects it outright — so an unconditional call here + // failed the *connect* of every modern server advertising `logging`, + // taking down commands that have nothing to do with logging. Modern has + // no session-scoped level at all: the equivalent is the per-request + // `io.modelcontextprotocol/logLevel` `_meta` opt-in, which + // `modernLogLevel` already drives from the server settings (see + // {@link setModernLogLevel}). So the right behavior on modern is to skip + // this call rather than translate it — `initialLoggingLevel` names a + // mechanism that era does not have. + if ( + this.initialLoggingLevel && + this.capabilities?.logging && + this.protocolEra !== "modern" + ) { await this.client.setLoggingLevel( this.initialLoggingLevel, this.getRequestOptions(), From 80eb321b1436fb324714984e68f228e305b1f9f1 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 15 Aug 2026 17:49:09 -0400 Subject: [PATCH 14/25] fix: stop const-less anyOf branches from crashing the form, and never select a non-string enum MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #2007. Two things, both surfaced while answering "does #1928's PR also fix #2007?". **#2007, and why it belongs in this PR.** `z.array(z.union([z.object(…), z.object(…)]))` gives an `items.anyOf` whose branches carry no top-level `const`, so every MultiSelect option was `String(item.const ?? "")` — the empty string, twice. Mantine *throws* on duplicate option values, greying out the whole tool panel. That is a pre-existing bug, but this branch makes it reachable from a new direction: a *nullable* array of object unions previously had no top-level `type` and fell to the JSON fallback, and now collapses to `type: "array"` with `items.anyOf` — straight into the crash. Shipping the collapse without this fix would widen an existing crash, so the two go together. Both shapes are covered by tests. `toConstOptions` now returns `null` unless every branch yields a distinct, non-empty value, and the field falls through to the JSON editor — which is the honest widget for a union of object shapes anyway. Applied to the `string`/`oneOf` path as well, which had the identical `?? ""` and the same duplicate-throw waiting on it. **Copilot round 2.** Round 1's fix kept "an explicit `type` is authoritative", so `{ type: "number", enum: [1, 2] }` still collapses with its enum hoisted. The TUI's dispatcher tests `property.enum` *before* the type switch, and `toSelectOptions` stringifies, so that reached a select that submits `"1"` for `1`. The select path is now gated on `isStringEnum` (exported from core for this), and a typed non-string enum falls through to its typed field: the enum constraint is lost, the value's type is not, which is the safer of the two losses. Pre-existing for a plain numeric enum; the collapse just made the nullable form reachable too. Signed-off-by: cliffhall --- clients/tui/__tests__/schemaToForm.test.ts | 33 ++++++ clients/tui/src/utils/schemaToForm.ts | 16 ++- .../groups/SchemaForm/SchemaForm.test.tsx | 102 ++++++++++++++++++ .../groups/SchemaForm/SchemaForm.tsx | 63 +++++++++-- core/json/nullableUnion.ts | 9 +- 5 files changed, 205 insertions(+), 18 deletions(-) diff --git a/clients/tui/__tests__/schemaToForm.test.ts b/clients/tui/__tests__/schemaToForm.test.ts index 93ca1ca9..769c3026 100644 --- a/clients/tui/__tests__/schemaToForm.test.ts +++ b/clients/tui/__tests__/schemaToForm.test.ts @@ -311,6 +311,39 @@ describe("schemaToForm", () => { ]); }); + // 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( { diff --git a/clients/tui/src/utils/schemaToForm.ts b/clients/tui/src/utils/schemaToForm.ts index 5201abbb..6332450c 100644 --- a/clients/tui/src/utils/schemaToForm.ts +++ b/clients/tui/src/utils/schemaToForm.ts @@ -3,7 +3,10 @@ */ import type { FormStructure, FormSection, FormField } from "ink-form"; -import { normalizeNullableUnion } from "@inspector/core/json/nullableUnion.js"; +import { + isStringEnum, + normalizeNullableUnion, +} from "@inspector/core/json/nullableUnion.js"; /** Minimal JSON Schema property shape used when building tool parameter forms */ interface JsonSchemaProperty { @@ -105,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/src/components/groups/SchemaForm/SchemaForm.test.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx index 3ff41b50..e3bc2b3c 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx @@ -971,6 +971,108 @@ describe("SchemaForm nullable unions", () => { 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"); + }); + + 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(); + }); + it("still falls back to the JSON input for a union of two real types", () => { const schema: InspectorFormSchema = { type: "object", diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx index 628f2ee3..9c203901 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx @@ -11,7 +11,10 @@ import { import { 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 { normalizeNullableUnion } from "@inspector/core/json/nullableUnion.js"; const FieldLabel = Text.withProps({ @@ -52,6 +55,44 @@ 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. Three ways that happens, all of which + * Mantine reacts to badly: + * + * - **No `const`.** 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 (branch.const === undefined || branch.const === null) { + return null; + } + const value = String(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 @@ -248,11 +289,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 (