diff --git a/.gitattributes b/.gitattributes index cf968721..056e3cb9 100644 --- a/.gitattributes +++ b/.gitattributes @@ -1 +1,2 @@ +* text=auto eol=lf package-lock.json linguist-generated=true diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 0bc70bfc..32122205 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -100,7 +100,7 @@ Both exist and do different jobs. Theme files (`src/theme/.ts`) custo ## Gates and PR hygiene - `npm run format` before committing; **`npm run ci` before pushing** (`validate` → `coverage` → `verify:build-gate` → `smoke` → Storybook). `npm run validate` is the fast inner-loop check and is **not** a substitute — it runs `test`, not `test:coverage`, so it does zero coverage gating. -- **A dependency bump must land in every install that declares it.** v2 is not a workspace — the root and each `clients/*` have their own `node_modules`, and a client's test project compiles `core/` and `test-servers/src` (which resolve from the **root**) alongside the client's own sources. Bumping a shared dependency in one manifest only puts two versions of it in one `tsc` program; for a recursive-generic surface like zod that exhausts the tsc heap (#1896). `verify:dep-lockstep` fails the build on this, so a PR bumping a package that the shared sources import should update the root **and every client that already lists it** — not every client unconditionally, since a package absent from an install can't skew and adding it there would be a spurious dependency. +- **A dependency bump must land in every install that declares it.** v2 is not a workspace — the root and each `clients/*` have their own `node_modules`, and a client's test project compiles `core/` and `test-servers/src` (which resolve from the **root**) alongside the client's own sources. Bumping a shared dependency in one manifest only puts two versions of it in one `tsc` program; for a recursive-generic surface like zod that exhausts the tsc heap (#1896). `verify:dep-lockstep` fails the build on this — it derives its candidates from what each `tsc` program actually resolves (`tsc --listFilesOnly`, keeping packages that reach one program from two installs), so a package reached only through another package's `.d.ts` counts too (#1965) — so a PR bumping a package the shared sources pull in should update the root **and every client that already lists it** — not every client unconditionally, since a package absent from an install can't skew and adding it there would be a spurious dependency. - **A test or smoke must not touch real user state.** The web smokes run against a throwaway catalog via the shared `scripts/lib/prod-web-server.mjs` helper, never the developer's `~/.mcp-inspector/mcp.json` (#1977); the cli/tui smokes drive a temp `--catalog`. A new smoke spawning its own server, or teardown that removes a work dir without first awaiting `stopChild` (the #1801 race — `child-cleanup.mjs` exports both halves and both are required), should be flagged. - **Every PR references an issue**, first body line `Closes #`. - **Every PR carries exactly one version label**, `v1` or `v2`, matching its base branch. diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index d1763cba..f7ddb49a 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -23,10 +23,10 @@ jobs: steps: - name: Checkout code - uses: actions/checkout@v6 + uses: actions/checkout@v7 - name: Setup Node.js - uses: actions/setup-node@v6 + uses: actions/setup-node@v7 with: node-version: '22.x' cache: 'npm' @@ -79,7 +79,7 @@ jobs: # smoke reuse the cache instead of downloading its own copy. The Storybook # step below reuses the same install. - name: Cache Playwright browsers - uses: actions/cache@v4 + uses: actions/cache@v6 with: path: ~/.cache/ms-playwright key: playwright-${{ runner.os }}-${{ hashFiles('clients/web/package-lock.json') }} @@ -129,10 +129,10 @@ jobs: id-token: write steps: - name: Checkout code - uses: actions/checkout@v6 + uses: actions/checkout@v7 - name: Setup Node.js - uses: actions/setup-node@v6 + uses: actions/setup-node@v7 with: node-version: '22.x' cache: 'npm' @@ -217,7 +217,7 @@ jobs: id-token: write steps: - name: Checkout code - uses: actions/checkout@v6 + uses: actions/checkout@v7 - name: Log in to the Container registry uses: docker/login-action@v4 @@ -241,14 +241,14 @@ jobs: latest=true - name: Set up QEMU - uses: docker/setup-qemu-action@v3 + uses: docker/setup-qemu-action@v4 - name: Set up Docker Buildx uses: docker/setup-buildx-action@v4 - name: Build and push Docker image id: push - uses: docker/build-push-action@v6 + uses: docker/build-push-action@v7 with: context: . push: true diff --git a/AGENTS.md b/AGENTS.md index 21061009..d07e84f4 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), @@ -35,7 +38,14 @@ v2/main/ │ │ # issuerBinding.ts SEP-2352 callback-leg failure │ │ # classification — separates a recoverable │ │ # "lost authorization state" from a genuine -│ │ # cross-AS issuer mismatch — #1808) +│ │ # cross-AS issuer mismatch — #1808; +│ │ # authorizationParams.ts per-server custom +│ │ # authorization-request parameters — the +│ │ # reserved-key list plus the merge applied in +│ │ # providers.redirectToAuthorization, the only +│ │ # seam that sees the SDK-built authorize URL; +│ │ # authorization request only, never the token +│ │ # request — #2018) │ │ ├── browser/ # Browser-side OAuth (sessionStorage, BrowserNavigation) │ │ ├── node/ # Node-side OAuth (NodeOAuthStorage, OAuthCallbackServer, │ │ │ # runner-interactive-oauth loopback callback flow) @@ -52,12 +62,24 @@ 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 │ │ # extension wire schemas + normalize/handle helpers, -│ │ # used by the raw-wire tasks/* channel — #1631) +│ │ # used by the raw-wire tasks/* channel — #1631; +│ │ # listSalvage.ts: per-item salvage for list results — +│ │ # keeps the valid entries when one is non-conforming +│ │ # instead of losing the whole list, and owns the +│ │ # shared isClientDecodeRejection predicate — #1909) │ │ ├── import/ # Config import strategies (#1348): client-config parsers │ │ │ # (Claude Desktop/Cursor/Cline/VS Code), registry │ │ │ # server.json parser, strategy registry + well-known @@ -96,8 +118,11 @@ v2/main/ ├── scripts/ # Root build/verify tooling: install-clients.mjs (the │ # postinstall cascade), the smoke-*.mjs runners, │ # verify-build-gate / verify-format-coverage / -│ # verify-typecheck-coverage, pack-and-verify.mjs, -│ # and lib/ shared helpers. Prettier-gated via +│ # verify-typecheck-coverage / verify-dep-lockstep, +│ # pack-and-verify.mjs, and lib/ shared helpers +│ # (tsc-program.mjs is the `tsc --listFilesOnly` +│ # measurement both coverage guards read a program +│ # through — #1965). Prettier-gated via │ # `format:check:scripts`; its own pure parsers are │ # unit-tested by `npm run test:scripts` (node --test). ├── specification/ # Build specification @@ -462,6 +487,10 @@ Don't lean on GitHub's permission gate to enforce this. Whether an outside repor So v2 flows `feature branch → v2/main → (milestone) main → npm latest`, while v1 is flat: `feature branch → v1/main → npm v1-latest`, with no merge into `main` at any point. The two lines are published independently under separate dist-tags, which is why a v1 fix does **not** need to be forward-ported to reach users on v1 (`npx @modelcontextprotocol/inspector@v1-latest`). + ⚠️ **The version bump rides that same flow — it is made on `v2/main`, never on the milestone-merge branch (#2010).** `npm version --no-git-tag-version` lands on `v2/main` *before* the milestone merge, so the bump reaches `main` as part of the milestone's work; the release tag (bare `x.y.z`, no `v` prefix) is then applied to the resulting **`main`** commit. This deliberately leaves a normal in-flight interval where `v2/main` reads the version being built and `main` the one currently released — that difference is expected. The invariant it buys is that they converge when the release merge lands, so `v2/main` is never left *behind* `main` afterwards. Bumping on the milestone-merge branch instead (which is cut from `main`) puts the bump only downstream of `v2/main`, where nothing carries it back — that is how `v2/main` sat at `2.0.0` through both the 2.1.0 and 2.2.0 releases, and how #2009 came to carry an unrelated `2.0.0 → 2.2.0` diff simply because its branch was cut from a milestone-merge branch. **Never close a drift by merging `main` into `v2/main`**: `main` holds the whole pre-v2 v1 history (~230 commits `v2/main` lacks), so a back-merge grafts all of it into the develop branch permanently to deliver a two-file change. Full procedure in the root README's [Cutting a release](./README.md#cutting-a-release). + + A corollary for **branching**: cut feature branches from **`v2/main`**, never from a milestone-merge branch — the latter carries release-only commits that will show up in your PR's diff. + - **Project Boards**: - v2 - https://github.com/orgs/modelcontextprotocol/projects/28 (active board — all new work goes here) - v1 - https://github.com/orgs/modelcontextprotocol/projects/11 (legacy inspector version, no new activity except security fixes) @@ -660,12 +689,12 @@ The ⚠️ option-deletion hazard, the snapshot rule, and the recovery recipe ab - ALWAYS do `npm run format` before committing — the **root** `format` auto-fixes `core/` (`format:core`), the root `scripts/` tooling (`format:scripts`), the root "shared" surface (`format:shared` — `test-servers/src/**`, `vitest.shared.mts`, the root `eslint.config.js`), and every client's scope in one shot. Every **client** format glob uses the uniform extension set `*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}` (#1792) so a new-extension file can't slip the gate; `core/` stays `{ts,tsx}` and the shared surface `{ts,tsx,mts,cts}` (their surfaces can't hold the other extensions), and `npm run verify:format-coverage` (the first step of `validate`, #1792) is the backstop — it fails if any tracked source file is left uncovered by a `format:check` glob regardless of which glob was expected to catch it. `validate` runs `format:check` (the non-fixing variant, including `format:check:core`, `format:check:scripts`, and `format:check:shared`) and will fail in CI on any unformatted file, so always run the auto-fixer first rather than letting `format:check` catch it. - **`npm run ci` is the mandatory pre-push command** — it mirrors `.github/workflows/main.yml` (minus `npm install`): `validate` → `coverage` → `verify:build-gate` (the #1769 browser-externalized-builtin build gate) → `smoke` → Storybook play-function tests (installs Playwright chromium if needed). It now runs **`npm run coverage`**, the per-file ≥90 gate (lines/statements/functions/branches) that CI enforces — so `npm run ci` is a true superset of GitHub CI, and passing it locally means CI's gates will pass. Expect several minutes. **`npm run validate`** remains the fast inner-loop check during development (unit tests only — no coverage gate, no smoke, no Storybook), but it is **NOT** an acceptable substitute for `npm run ci` before pushing: `validate` runs `test`, not `test:coverage`, so it does **zero** coverage gating. Skipping the gate is how a push passes every fast local check and still fails CI (this exact gap broke PR #1601 on a function-coverage regression). -- ALWAYS do `npm run format` before committing, then **`npm run ci`** before pushing. From the repo root, `validate` runs **`verify:format-coverage` first** (the #1792 guard — asserts every tracked source file is covered by a `format:check` glob), then **`verify:typecheck-coverage`** (the #1791 guard — asserts every tracked `.ts`/`.tsx`/`.mts`/`.cts` in each gated Node client, plus the non-client first-party TS like `core/` and `test-servers/src`, lands in a tsconfig project), then **`verify:dep-lockstep`** (the #1896 guard — asserts no dependency *directly imported* by the shared sources resolves to two different versions across installs; transitive-only declarations are a known boundary, #1965), then **`test:scripts`** (the guards' own parser unit tests, `node --test`), then the **`core/` gate** (`validate:core`), then chains the four per-client validations (`validate:web` → `validate:cli` → `validate:tui` → `validate:launcher`); each client delegates to its own `npm run validate` in its own folder (no coverage — fast). Every client is self-validating and the top level just chains them, building each client's bundle along the way (no cross-client build dependencies). +- ALWAYS do `npm run format` before committing, then **`npm run ci`** before pushing. From the repo root, `validate` runs **`verify:format-coverage` first** (the #1792 guard — asserts every tracked source file is covered by a `format:check` glob), then **`verify:typecheck-coverage`** (the #1791 guard — asserts every tracked `.ts`/`.tsx`/`.mts`/`.cts` in each gated Node client, plus the non-client first-party TS like `core/` and `test-servers/src`, lands in a tsconfig project), then **`verify:dep-lockstep`** (the #1896 guard — asserts no dependency that reaches a single `tsc` program from two installs resolves to two different versions across them), then **`test:scripts`** (the guards' own parser unit tests, `node --test`), then the **`core/` gate** (`validate:core`), then chains the four per-client validations (`validate:web` → `validate:cli` → `validate:tui` → `validate:launcher`); each client delegates to its own `npm run validate` in its own folder (no coverage — fast). Every client is self-validating and the top level just chains them, building each client's bundle along the way (no cross-client build dependencies). - **`validate:core` is the root-owned format + lint gate (#1689, widened in #1778 and #1767).** Each client's `prettier`/`eslint` is scoped to its own dir, so nothing reached `core/`, the root `scripts/`, or the root "shared" surface before — `validate:core` closes that: it runs `format:check:core` (`prettier --check "core/**/*.{ts,tsx}"`) + `format:check:scripts` (`prettier --check "scripts/**/*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}"`, the root build/verify tooling — #1778) + `format:check:shared` + `lint:core` (`eslint "core/**/*.{ts,tsx}"` via the **root** `eslint.config.js`) + `lint:shared`. Use `npm run format:core` / `npm run format:scripts` / `npm run format:shared` to auto-fix (all folded into the root `format`). The **shared surface** (#1767) is `test-servers/src/**/*.{ts,tsx,mts,cts}`, the root `vitest.shared.mts`, and the root `eslint.config.js` — first-party code no client's `eslint .` / `prettier` reaches; it is both prettier-gated (`format:check:shared`) and eslint-gated (`lint:shared`, via a second `files` block in the root `eslint.config.js` scoped to Node globals). The `scripts/` gate is prettier-only — the root has no eslint config for `.mjs`. The root carries prettier/eslint as devDependencies for this; `core/` is isomorphic (browser + Node globals, no JSX today — the `{ts,tsx}` glob future-proofs against a `core/**/*.tsx`). The root `eslint.config.js` honors an `_`-prefix as the intentionally-unused marker (`argsIgnorePattern`/`varsIgnorePattern`/`caughtErrorsIgnorePattern: '^_'`). **prettier is pinned to an exact version** (not a caret) in all five `package.json`s (#1790) so the gate's verdict can't shift with an in-range patch bump. - **cli and tui now typecheck their `src` (#1689).** Their `build`/`test` run through esbuild (no type check), so each has a `typecheck` script folded into `validate`. Their `tsconfig.json` matches `clients/web/tsconfig.app.json`'s module/lib _resolution_ options — DOM lib, `moduleResolution: bundler`, and **no** `noUncheckedIndexedAccess` (web's app config does not extend `tsconfig.base`, so re-enabling it would surface `core/` issues web never gates) — so the imported `core/` sources are validated the same way web validates them. It does **not** mirror web's extra strictness flags (`noUnusedLocals`, `verbatimModuleSyntax`, ES2023 target, …), so cli/tui's own `src` is checked slightly more loosely than web's. `core/` itself still typechecks through web's `tsc -b`. - **The `__tests__` dirs are typechecked too (#1791).** The src-only `tsconfig.json` excludes `**/*.test.*`, so each of cli, tui, and launcher carries a **`tsconfig.test.json`** — extending the build config, `noEmit`, including `__tests__/**/*` (only the tests root the project; tsc pulls in the `src` they import, and the src-only config already validates all of `src` without the test-only aliases) and adding the test-only path aliases that resolve what vitest resolves via `vitest.shared.mts`. The alias set differs per client: **cli's is the widest** (`@modelcontextprotocol/inspector-test-server` → `test-servers/src`, the `@inspector/core/*` deep paths, express/vitest — cli is the only one importing the test-server package); **tui's** carries only the `@inspector/core/*` + react/vitest redirects; **launcher's** has **no** `paths` at all — it's a plain `rootDir: "."` sibling of the build config (whose `rootDir: ./src` is what rejects the tests). Each client's `typecheck` script runs **both** projects (`tsc -p tsconfig.json && tsc -p tsconfig.test.json`) so running it standalone means the same thing everywhere (launcher's `build` also `tsc`s `src`, but `typecheck` doesn't rely on that). cli additionally carries `@types/express` (devDep) so the transitively-aliased test-server source typechecks, mirroring `clients/web` (cli's `tsconfig.test.json` also names `test-servers/src/server-composable.ts` explicitly — a bin entry the barrel doesn't import, so nothing else gives it a tsc pass). The client **config files** are typechecked too: cli's/tui's (`vitest.config.ts`, `tsup.config.ts`, tui `dev.ts`) are folded into each src `tsconfig.json`'s `include`; launcher's `vitest.config.ts` goes in its `tsconfig.test.json` instead (again the `rootDir: ./src` reason). Note the gate checks mock **implementations and return types** (typing a `vi.fn()` against a real signature keeps its `mockResolvedValue`/impl in sync) but **not** `toHaveBeenCalledWith(...)` arguments — vitest types those to accept anything regardless of the mock's type parameter. **`npm run verify:typecheck-coverage`** (`scripts/verify-typecheck-coverage.mjs`, run as the second step of `validate` right after `verify:format-coverage`) is the durable guard for this invariant: it runs each client's `typecheck` projects with `tsc --listFilesOnly`, unions them, and fails on any tracked `.ts`/`.tsx`/`.mts`/`.cts` that lands in no project — for every gated Node client, which it discovers from disk (each `clients/*` is enrolled through its `typecheck` script's projects, or — for a `tsc -b` client like `clients/web` with no `typecheck` script — through its `tsconfig.json` `references`), so a new client is covered without editing the guard — the typecheck analog of `verify:format-coverage`, since a project only reaches the files its `include` names plus their transitive imports, so a new top-level file (launcher especially, whose build `rootDir: ./src` rejects package-root files) can otherwise fall out silently. Like its sibling it also asserts the gate is _wired_ (each client's typecheck pass is reachable from its `validate` — its `typecheck` script for cli/tui/launcher, or a real `tsc -b` for web — and the root chain runs each client's `validate`), so it can't stay green while measuring a pass nothing invokes. It asserts the same of **`test:scripts`** — its own parser tests — on three axes: reachable from the root `validate`, a **non-empty** tracked `scripts/**/*.{test,spec}.*` set, and **every one of those files matched by a glob harvested across the scripts reachable from `test:scripts`** (so a delegating `test:scripts` still measures correctly). The third axis exists because `node --test` silently _skips_ a file its glob misses and still exits 0 — a rename to `*.spec.mjs` would shrink the suite with a green run. Beyond the clients it also covers, **deny-by-default**, the first-party TS no client owns — everything tracked outside `clients/*` (`test-servers/src/**`, the root `vitest.shared.mts`, **all of `core/`**, and any new top-level TS location) must land in the _global_ union of client projects (cli aliases the test-server source; web's enrolled projects include `core/`). So a `core` `*.tsx` web's `include` doesn't reach, or an unimported `test-servers/src` bin entry, can't ship uncompiled-but-unchecked. The one "listed but unchecked" tier the guard structurally can't see — a per-file `// @ts-nocheck` — is owned by a different gate: `@typescript-eslint/ban-ts-comment` rejects it across every surface (`lint:core`, `lint:shared`, and each client's `eslint .`). The guard's own pure parsers (`scripts/lib/npm-scripts.mjs` + the exported helpers of `verify-typecheck-coverage.mjs`, whose execution is behind a `main()` so importing it for tests doesn't run it) are **unit-tested** — `npm run test:scripts` (node's built-in `node --test`, in `validate`; the root has no vitest harness by design) runs table-driven cases, one per rule the guard's parsers encode, and the guard itself enforces that this stays wired (above). - The one CLI nuance: `clients/cli`'s out-of-process `e2e.test.ts` spawns the built binary, so its `test` **builds first** via `pretest` (`test-servers:build && build`). To avoid building it twice, `clients/cli`'s `validate` folds that in — it is `format:check && lint && typecheck && test` with **no** separate `build` step (the other clients, whose tests don't spawn their bundle, keep an explicit `build`). `validate:web`/`validate:tui`/`validate:launcher` are the uniform `format:check && lint && (typecheck &&) build && test`. (#1778, #1789, #1792) `clients/web`'s `format`/`format:check` covers `src`, `server`, `.storybook`, and its top-level configs (the uniform `*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}` glob — `vite.config.ts`, `tsup.runner.config.ts`, `eslint.config.js`, …), not just `src`, so the Node backend, Storybook config, and Vite/build config are prettier-gated too; `clients/launcher`'s covers `src`, `__tests__`, `scripts`, and its top-level configs (the `*.` top-level glob is non-recursive, so each nested dir — `.storybook`, `scripts` — is named explicitly). The `verify:format-coverage` guard (#1792) enforces that this coverage stays complete. - - **One version per install-crossing dependency (#1896).** Because v2 is not a workspace, the root and each `clients/*` carry their own `node_modules` — and a client's `tsconfig.test.json` compiles first-party sources that live *outside* the client (`test-servers/src`, `core/`), which resolve their dependencies from the **root** install while the client's own sources resolve from the client install. So the same package can appear **twice in one `tsc` program**. At the same version that duplication is harmless; on a skew, TypeScript must relate two structurally-distinct declarations of the same type. For a deeply recursive-generic surface that is exponential: zod `4.3.6` (root) against zod `4.4.3` (`clients/web`) made `clients/web`'s `tsc -b` exhaust the 4GB default heap outright via `TS2589 Type instantiation is excessively deep`, because every `@modelcontextprotocol/*` schema is built out of zod generics. **Raising the heap with `--max-old-space-size` hides this class rather than fixing it — align the versions instead.** `npm run verify:dep-lockstep` (`scripts/verify-dep-lockstep.mjs`, in `validate`) is the durable guard: it **derives** the candidate set from the packages the shared first-party TypeScript imports — `core/`, `test-servers/src`, and the individually-named `vitest.shared.mts` (root-owned and imported by every client's vitest config) — so a new shared dependency is covered without editing the guard. It reads the committed lockfiles' **top-level** `node_modules/` entries — a *nested* transitive duplicate inside one install is routine and deliberately ignored — and fails, **deny-by-default**, on any candidate held at two versions across installs. The escape hatch is `TOLERATED_SKEW` in that file, an allowlist of *names* (not version pairs, so an ordinary patch float doesn't churn it), each entry carrying why that package's types can't blow up; `react`, `hono`, `jose`, and `@modelcontextprotocol/ext-apps` are listed today. **Being listed is not a blanket exemption** — it tolerates skew only *within a major version*, since a rationale about patch-level differences says nothing about a React 18-vs-19 split, where the type surface itself changes; a cross-major skew fails even for a listed package. **When bumping a dependency that the shared sources import, bump it in every install that declares it** — that's the root plus whichever clients list it, not all four unconditionally (launcher declares no zod, for instance, and a package absent from an install can't skew, so the guard ignores it there). Don't add a dependency to a client just to satisfy this. Its pure helpers are unit-tested via `test:scripts`, and it vouches — with `verify:format-coverage` and `verify:typecheck-coverage` — that its siblings are still wired into `validate`. + - **One version per install-crossing dependency (#1896).** Because v2 is not a workspace, the root and each `clients/*` carry their own `node_modules` — and a client's `tsconfig.test.json` compiles first-party sources that live *outside* the client (`test-servers/src`, `core/`), which resolve their dependencies from the **root** install while the client's own sources resolve from the client install. So the same package can appear **twice in one `tsc` program**. At the same version that duplication is harmless; on a skew, TypeScript must relate two structurally-distinct declarations of the same type. For a deeply recursive-generic surface that is exponential: zod `4.3.6` (root) against zod `4.4.3` (`clients/web`) made `clients/web`'s `tsc -b` exhaust the 4GB default heap outright via `TS2589 Type instantiation is excessively deep`, because every `@modelcontextprotocol/*` schema is built out of zod generics. **Raising the heap with `--max-old-space-size` hides this class rather than fixing it — align the versions instead.** `npm run verify:dep-lockstep` (`scripts/verify-dep-lockstep.mjs`, in `validate`) is the durable guard: it **derives** the candidate set from **what actually enters each `tsc` program** (#1965) — every client tsconfig project is listed with `tsc --listFilesOnly` through the shared `scripts/lib/tsc-program.mjs` helper (the same machinery `verify:typecheck-coverage` reads a program through, so the two guards can't disagree about what one contains), each resolved `node_modules` file is mapped to its owning install and package, and a package reaching **one** program from **two** installs is a candidate. That is precisely the set that can put two structurally-distinct copies of a type in front of one checker — and it needs no editing when a new dependency arrives. It replaced a derivation that read the packages the shared sources named *directly*, which could not see one whose declarations arrive only through another package's `.d.ts`: `@modelcontextprotocol/sdk` is never written in first-party code (the shared sources import the split `@modelcontextprotocol/client|core|…`) yet 16 of its `.d.ts` files land in `clients/web`'s test program, so a second copy under `clients/web/node_modules` skewed unseen. Two properties are worth knowing: a package present in two installs but reached from only one in a given program is correctly **not** a candidate, and TypeScript's package-identity redirect collapses two copies at the *same* name@version (so an aligned package's own transitive dependencies load once) — the redirect stops applying the moment they skew, which is exactly when the guard needs to see both. Versions still come from the committed lockfiles, but from the entry for the **exact install path the program resolved** (`node_modules/zod`, `node_modules/a/node_modules/zod`) rather than from the install's top-level entry: a *nested* duplicate inside one install is still not a candidate on its own — folding it onto its outermost install is what keeps the set small — but once a program has loaded one, pricing it from a top-level entry that may be absent or differently versioned would let a real pair pass (Copilot). Only the installs that actually **met in one program** are compared, so a third install's copy that no program loads beside another is not evidence of anything. Any co-occurrence whose copies disagree fails, **deny-by-default**; a resolved copy with no lockfile entry fails too, since the tree and the lockfile then disagree about what was loaded. The escape hatch is `TOLERATED_SKEW` in that file, an allowlist of *names* (not version pairs, so an ordinary patch float doesn't churn it), each entry carrying why that package's types can't blow up; it is **empty today** — the four names it used to carry (`react`, `hono`, `jose`, `@modelcontextprotocol/ext-apps`) were admitted under the old derivation and none is a candidate under this one, so each would be a rationale for a skew that cannot occur. **Being listed is not a blanket exemption** — it tolerates skew only *within a major version*, since a rationale about patch-level differences says nothing about a React 18-vs-19 split, where the type surface itself changes; a cross-major skew fails even for a listed package. **When bumping a dependency that the shared sources pull in, bump it in every install that declares it** — that's the root plus whichever clients list it, not all four unconditionally (launcher declares no zod, for instance, and a package absent from an install can't skew, so the guard ignores it there). Don't add a dependency to a client just to satisfy this. Its pure helpers are unit-tested via `test:scripts` (its own, plus `scripts/lib/tsc-program.test.mjs` for the shared derivation), and it vouches — with `verify:format-coverage` and `verify:typecheck-coverage` — that its siblings are still wired into `validate`. It runs the same `tsc --listFilesOnly` pass its sibling does, in its own process, costing ~14s: the listing is deliberately **not** cached to disk between the two, because a fingerprint that missed an input would make a guard measure a program that no longer exists and pass on a real miss. - **`npm run coverage`** is the per-file ≥90 gate and is now part of `npm run ci` — never treat it as optional before a push. It supersedes the old standalone `test:integration` step: web's `test:coverage` runs the `unit` **and** `integration` projects under v8 instrumentation, so `coverage` both enforces the ≥90 gate and exercises the same web integration paths CI covers. - **`smoke` is NOT part of `validate`** — it is included in `npm run ci`. It runs `smoke:launcher` (`--help` dispatch) plus the prod `smoke:cli` / `smoke:tui` / `smoke:web` / `smoke:web:browser` / `smoke:web:app`, and contains **no build commands** — it assumes the cli/tui/launcher bundles already exist (a full `validate` builds them; `smoke:web` builds `clients/web/dist` on demand). CI runs `validate`, then the `coverage` gate (which also covers the web integration project), then `verify:build-gate` (the #1769 build gate — see below), then `smoke` (with Playwright chromium installed just before it, since `smoke:web:browser` needs it). GitHub CI runs this same chain as separate workflow steps, with the Storybook play-function tests last (see below). - `smoke:launcher` (`scripts/smoke-launcher.mjs`) runs the built launcher with `--help`, `--cli --help`, and `--tui --help`, asserting each exits 0 and prints that mode's usage banner (which also proves the launcher resolved and loaded the right client build). It's the cheap dispatch check before the heavier prod smokes below. 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..cb57dd35 100644 --- a/README.md +++ b/README.md @@ -32,7 +32,8 @@ inspector/ ├── core/ # Shared code consumed via the `@inspector/core` alias (no package.json) │ ├── auth/ # OAuth: providers, discovery, storage, mid-session recovery (browser/node/remote backends) │ ├── client/ # Install-level client config (`client.json`): browser-safe parse/validate + Node load/save, remote backend, secrets -│ ├── json/ # JSON + parameter/argument conversion utilities +│ ├── json/ # JSON + parameter/argument conversion utilities, and the nullable-union +│ │ # schema collapse shared by the web and TUI form builders │ ├── logging/ # Silent pino logger singleton │ ├── mcp/ # InspectorClient runtime, state stores, transports, config import │ ├── node/ # Node-only shared helpers: version reader, hostUrl (host normalize/canonicalize + all-interfaces/loopback detection) @@ -145,6 +146,7 @@ Each config below is a ready-made server for exercising one feature by hand. Loa | `pagination-http.json` | Page-by-page list fetching | [#1721](https://github.com/modelcontextprotocol/inspector/issues/1721) | | `structured-output-http.json` | Tools tab: a result's `structuredContent` section | [#1908](https://github.com/modelcontextprotocol/inspector/issues/1908) | | `duplicate-tool-names-http.json` | A `tools/list` that repeats a tool name | [#1957](https://github.com/modelcontextprotocol/inspector/issues/1957) | +| `nullable-fields-http.json` | Tools tab: nullable (`anyOf` + `null`) arguments | [#1928](https://github.com/modelcontextprotocol/inspector/issues/1928) | | `advertised-extensions-http.json` | Tool registration gated on advertised extensions | [#1739](https://github.com/modelcontextprotocol/inspector/issues/1739) | | `logging-{legacy,modern}-http.json` | Logging, both eras | [#1629](https://github.com/modelcontextprotocol/inspector/issues/1629) | | `subscriptions-{legacy,modern}-http.json` | Resource subscriptions, both eras | [#1630](https://github.com/modelcontextprotocol/inspector/issues/1630) | @@ -227,6 +229,14 @@ Connect (default legacy era), open the Tools tab, and type `get` into **Search t The duplicated copies are appended rather than placed beside their twin on purpose. React matches a leading run of same-key children first, so a head-adjacent duplicate happens to line up and the defect hides; separating the pair is what makes it observable — and it is also the realistic shape, two tool sources concatenated. +#### Nullable arguments + +`nullable-fields-http.json` serves `record_shipment`, whose four arguments are each declared with Zod's `.nullish()` — "optional **and** explicitly nullable". That compiles to `anyOf: [, { "type": "null" }]`, so the real type (and, for the enum, its `enum` list) sits on a branch rather than at the top level. `get_temp` sits alongside it with a plain, non-nullable `units` enum for comparison. Plain streamable-HTTP — connect with the **default (legacy)** protocol era. + +Open the Tools tab and select `record_shipment`: `direction` must render as a **Select** (`envio` / `recebimento`) with a clear button that sets it back to `null`, `reference` as a text input, `quantity` as a number input, and `express` as a checkbox. On the broken build every one of them fell through to the raw-JSON textarea, which re-escaped its own contents on each keystroke until the value was unusable ([#1928](https://github.com/modelcontextprotocol/inspector/issues/1928)). The tool echoes the arguments it received, so the result panel shows exactly what was sent. + +The **TUI** had the same gap and is worth checking against the same server (`--tui`, then test `record_shipment`): `direction` is a select, `quantity` an integer field, `express` a boolean. Both clients now share one collapse step — `normalizeNullableUnion` in [`core/json/nullableUnion.ts`](./core/json/nullableUnion.ts) — precisely so they cannot drift on which schemas they can render. + #### Advertised extensions `advertised-extensions-http.json` serves `echo` (always) and a `get_weather` tool **gated on the `io.modelcontextprotocol/tasks` extension** (`extensionGatedTools`): the tool is registered but starts disabled, and the server enables it on `notifications/initialized` only when the client declared that extension in its `capabilities.extensions`. @@ -284,14 +294,14 @@ Each client self-validates from its own folder; the root scripts chain them. The | Script | What it does | | ----------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `npm run validate` | Runs the three durable guards first — `verify:format-coverage` (every tracked source file is format-gated), `verify:typecheck-coverage` (every one lands in a tsconfig project), `verify:dep-lockstep` (no dependency the shared sources directly import skews across installs) — then `test:scripts` (the guards' own parser unit tests), then `validate:core` (the shared `core/` `format:check` + `lint` gate), then per client: `format:check` + `lint` + **`typecheck`** (cli/tui/launcher; web typechecks via `tsc -b` inside its `build`) + `build` + fast unit tests. The quick inner-loop check. | +| `npm run validate` | Runs the three durable guards first — `verify:format-coverage` (every tracked source file is format-gated), `verify:typecheck-coverage` (every one lands in a tsconfig project), `verify:dep-lockstep` (no dependency reaching one `tsc` program from two installs skews across them) — then `test:scripts` (the guards' own parser unit tests), then `validate:core` (the shared `core/` `format:check` + `lint` gate), then per client: `format:check` + `lint` + **`typecheck`** (cli/tui/launcher; web typechecks via `tsc -b` inside its `build`) + `build` + fast unit tests. The quick inner-loop check. | | `npm run coverage` | The **per-file ≥90% gate** (lines/statements/functions/branches) under v8 instrumentation, per client. CI-enforced. For web this also runs the integration project and covers the shared `core/` runtime (including `core/json` and `core/client`). | | `npm run smoke` | End-to-end smokes through the built launcher (`--help` dispatch + prod cli/tui/web), plus two headless-Chromium smokes: a boot smoke that runs the prod web bundle and asserts a clean first render (no uncaught error — sync exception or unhandled rejection, how a Node built-in reaching the browser bundle manifests), and an **MCP Apps** smoke (`smoke:web:app`) that drives connect → open app → `data-app-status="ready"` against a composable App server, covering the sandbox proxy and UI-protocol bridge. | | `npm run verify:build-gate` | Runs a real `vite build` with a Node built-in forced into the browser graph and asserts the build **fails** via the #1769 gate (which turns Vite's browser-externalization warning into a hard error). Guards against the warning phrasing drifting in a Vite bump and silently disabling the gate. Part of `npm run ci`. | | `npm run verify:format-coverage` | Parses the `format:check` globs out of every `package.json` (only those reachable from `validate`), enumerates all tracked source files, and **fails** listing any not covered by a glob — the durable guard for the "every first-party source file is format-gated" invariant (#1792). Runs first in `validate`. | -| `npm run test:scripts` | Table-driven unit tests (`node --test`) for the guard's own pure parsers (`scripts/lib/npm-scripts.mjs` + the exported helpers of `verify-typecheck-coverage.mjs` and `verify-dep-lockstep.mjs`), one case per rule they encode. Runs in `validate` — and `verify:typecheck-coverage` guards *this* gate in turn (reachable from `validate`, non-empty test set, every test file matched by the `test:scripts` glob), since `node --test` silently skips a file its glob misses and still exits 0. | +| `npm run test:scripts` | Table-driven unit tests (`node --test`) for the guard's own pure parsers (`scripts/lib/npm-scripts.mjs`, `scripts/lib/tsc-program.mjs` + the exported helpers of `verify-typecheck-coverage.mjs` and `verify-dep-lockstep.mjs`), one case per rule they encode. Runs in `validate` — and `verify:typecheck-coverage` guards *this* gate in turn (reachable from `validate`, non-empty test set, every test file matched by the `test:scripts` glob), since `node --test` silently skips a file its glob misses and still exits 0. | | `npm run verify:typecheck-coverage` | The typecheck-coverage analog of the above (#1791): for each Node client (auto-discovered from disk — enrolled via its `typecheck` script's projects, or for a `tsc -b` client like `clients/web` via its `tsconfig.json` `references`) it runs those projects with `tsc --listFilesOnly`, unions them, and **fails** listing any tracked `.ts`/`.tsx`/`.mts`/`.cts` under the client that lands in no project (so a new top-level config/helper can't silently go untypechecked). It also requires, deny-by-default, the first-party TS no client owns (`test-servers/src`, the root `vitest.shared.mts`, all of `core/`, and any new top-level location) to land in some client project's tsc pass — so a `core` `*.tsx` web's projects don't reach is caught too. Also asserts the gate is wired (each client's typecheck pass — its `typecheck` script, or web's `tsc -b` — is reachable from its `validate`, and the root chain runs each client's `validate`). Runs in `validate`. | -| `npm run verify:dep-lockstep` | Guards the "one version per install-crossing dependency" invariant (#1896). v2 is not a workspace, so a client's test project compiles the shared first-party TypeScript — `core/`, `test-servers/src`, and the root-owned `vitest.shared.mts`, all of which resolve their dependencies from the **root** install — alongside the client's own sources, putting the same package in one `tsc` program twice. At the same version that's harmless; skewed, TypeScript must relate two structurally-distinct copies of every type, which for a recursive-generic surface is exponential (zod `4.3.6` vs `4.4.3` exhausted the 4GB tsc heap in `clients/web`). Derives its candidate set from what those shared sources import, compares the committed lockfiles' top-level entries, and **fails deny-by-default** on any skew not in the annotated `TOLERATED_SKEW` allowlist — and an allowlisted package is tolerated only *within a major version*. Runs in `validate`. | +| `npm run verify:dep-lockstep` | Guards the "one version per install-crossing dependency" invariant (#1896). v2 is not a workspace, so a client's test project compiles the shared first-party TypeScript — `core/`, `test-servers/src`, and the root-owned `vitest.shared.mts`, all of which resolve their dependencies from the **root** install — alongside the client's own sources, putting the same package in one `tsc` program twice. At the same version that's harmless; skewed, TypeScript must relate two structurally-distinct copies of every type, which for a recursive-generic surface is exponential (zod `4.3.6` vs `4.4.3` exhausted the 4GB tsc heap in `clients/web`). Derives its candidate set from **what actually enters each program** (#1965) — every client tsconfig project listed with `tsc --listFilesOnly` via the shared `scripts/lib/tsc-program.mjs`, each resolved `node_modules` file mapped to its owning install, keeping the packages that reach one program from two installs (a package whose declarations arrive only through another package's `.d.ts`, as `@modelcontextprotocol/sdk`'s do, is invisible to a scan of first-party imports). Prices each copy from the lockfile entry for the exact install path the program resolved, compares only the installs that met in one program, and **fails deny-by-default** on any disagreement not in the annotated `TOLERATED_SKEW` allowlist — empty today — with an allowlisted package tolerated only *within a major version*. Runs in `validate`. | `npm run ci` | **Mandatory pre-push command.** `validate` → `coverage` → `verify:build-gate` → `smoke` → Storybook. A true superset of GitHub CI. | | `npm run pack:verify` | Publish smoke — see [Publishing](#publishing). | @@ -326,16 +336,42 @@ Publishing is automated by two release-gated jobs in [`.github/workflows/main.ym A v2 release is cut from **`main`**, after the milestone's work has been merged there from `v2/main` — not from `v2/main` itself. (The v1 line releases independently from `v1/main` to the `v1-latest` tag and never touches `main`; see [Repo status](#mcp-inspector).) -Because there is **one version number** (only the root `package.json` has one — the clients carry none, so there is nothing to keep in sync and no `check-version` step), the release flow is just: +Because there is **one version number** (only the root `package.json` has one — the clients carry none, so there is nothing to keep in sync and no `check-version` step), the release flow is three steps. + +**1. Bump on `v2/main`, before the milestone merge.** The bump is part of the milestone's work, so it belongs on the develop branch and flows into `main` with everything else: + +Substitute the real issue number and release below — the commands are written to be copy-pasteable as-is (a `2.2.0` → `2.3.0` minor bump): ```bash -npm version # bumps the root package.json + tags -git push --follow-tags +git checkout -b v2/chore/2010-bump-2-3-0 v2/main +npm version minor --no-git-tag-version # or major / patch; bump only, no tag +# PR → v2/main +``` + +⚠️ **`--no-git-tag-version` is load-bearing.** A bare `npm version` also tags, and the tag would land on a `v2/main` commit — but the release must be cut from `main`, so the tag has to point at the merge commit there (step 3). Tagging here creates a tag on a commit that is never released. + +**2. Merge `v2/main` → `main`** through the usual milestone-merge branch. It now carries the bump, so the release lands on `main` with the version already correct. + +Between steps 1 and 2 the two branches **do** differ, and that is expected, not drift: `v2/main` reads the version being built while `main` still reads the one currently released. What this ordering removes is *post-release* drift — once the milestone merge lands they agree again, and `v2/main` is never left **behind** `main`. If you see `v2/main` ahead of `main`, a release is in flight; if you see it behind, something went wrong. + +**3. Tag the `main` commit and draft the Release:** + +```bash +git fetch origin main +git tag 2.3.0 origin/main && git push origin 2.3.0 # then draft & publish a GitHub Release for that tag → triggers `publish` ``` +⚠️ **Tag `origin/main`, not your local `HEAD`.** `git checkout main && git pull` resolves through whatever merge-or-rebase strategy you have configured, so a divergent local `main` can quietly produce or replay local commits. Tagging `HEAD` there tags a commit that is not on `origin/main`, and `git push origin ` pushes only the tag — leaving a release whose commit was never published. Naming `origin/main` explicitly makes the tagged commit exactly what the remote branch points at, regardless of local state. + +⚠️ **No `v` prefix.** This repo's release tags are bare `x.y.z` — `2.2.0`, `2.1.0`, `2.0.0` — so tag `2.3.0`, not `v2.3.0`. Note npm's own `tag-version-prefix` defaults to `v` and the repo sets no `.npmrc`, so a bare `npm version` would have produced a `v`-prefixed tag that does not match the convention. Tagging by hand (step 3) is what keeps it right. The workflow's assert step strips a leading `v` before comparing, so a `v`-prefixed tag would still publish — it would just be inconsistent with every previous release. + The release's target commit selects which workflow runs, so this only publishes when a release is cut from a commit carrying this (v2) workflow. +**Why the bump goes on `v2/main` first ([#2010](https://github.com/modelcontextprotocol/inspector/issues/2010)).** It used to happen on the milestone-merge branch, which is cut from `main` — so the bump existed only *downstream* of `v2/main` and nothing carried it back. `v2/main` sat at `2.0.0` through both the 2.1.0 and 2.2.0 releases. That is not cosmetic: a branch cut from a milestone-merge branch silently carries the bump into an unrelated PR (this happened on [#2009](https://github.com/modelcontextprotocol/inspector/issues/2009), where a container bugfix arrived with a `2.0.0 → 2.2.0` diff), and anything reading the version in development — `readInspectorVersion()`, `--version`, `GET /api/config` — reported a version two releases old. + +Do **not** "fix" a future drift by merging `main` back into `v2/main`. `main` carries the entire pre-v2 v1 history (retained through `ec5d8e13 chore: replace main's tree with v2` — ~230 commits `v2/main` does not have), so a back-merge grafts all of it into the develop branch's log permanently in order to deliver a two-file change. Bumping first means there is nothing to back-merge. + ### Docker A container image is published to GHCR (`ghcr.io/modelcontextprotocol/inspector`, `linux/amd64` + `linux/arm64`) by the release workflow. The [`Dockerfile`](Dockerfile) is a two-stage build: the first stage installs and `npm pack`s the publishable tarball; the second stage `npm install -g`s that tarball, so the image ships the exact same artifact as npm, with a clean `mcp-inspector` bin. @@ -349,6 +385,15 @@ docker build -t mcp-inspector . docker run --rm -p 127.0.0.1:6274:6274 mcp-inspector ``` +**Using the Apps tab? Publish `6275` too.** The MCP Apps sandbox is a second listener the browser reaches directly, on `MCP_SANDBOX_PORT` (default `6275`). Nothing else needs it, so the single-port commands above are fine for ordinary inspection — but the Apps tab renders a blank widget without it: + +```bash +docker run --rm -p 127.0.0.1:6274:6274 -p 127.0.0.1:6275:6275 \ + ghcr.io/modelcontextprotocol/inspector +``` + +Publish it on the **same port number** inside and out. The sandbox URL is handed to the browser via `/api/config` as `http://localhost:/sandbox`, so remapping it (`-p 9000:6275`) advertises a port the browser can't reach; use `-e MCP_SANDBOX_PORT=9000 -p 127.0.0.1:9000:9000` instead. + **Keep the `127.0.0.1:` prefix on the published port.** A bare `-p 6274:6274` publishes on **every host interface**, putting the Inspector on your local network. The container's `HOST=0.0.0.0` is a separate concern — it governs the _container's_ interfaces, not the host's — so the `DANGEROUSLY_BIND_ALL_INTERFACES` opt-in that guards a wildcard bind outside a container does not cover this. It matters more here than for an ordinary web app: the backend spawns processes on request, `GET /` embeds the API token into the served HTML, and a request arriving with **no** `Origin` header skips the origin allow-list entirely — so for any non-browser client the API token is the only guard. Publishing wider needs a real access-control boundary in front of the Inspector — a reverse proxy that authenticates, an SSH tunnel, a private network. Setting your own `MCP_INSPECTOR_API_TOKEN` does **not** substitute: `GET /` discloses whatever token is in use, so a custom one is harvested exactly as easily as a generated one. **Keeping the servers you add.** The Inspector saves your server list to `$HOME/.mcp-inspector/mcp.json`, which in the image is `/home/node/.mcp-inspector/mcp.json` — inside the container's writable layer, so `--rm` discards it and every run starts with an empty list. Mount a volume there to keep it: diff --git a/clients/tui/__tests__/schemaToForm.test.ts b/clients/tui/__tests__/schemaToForm.test.ts index 7efca937..f47450e3 100644 --- a/clients/tui/__tests__/schemaToForm.test.ts +++ b/clients/tui/__tests__/schemaToForm.test.ts @@ -213,4 +213,173 @@ describe("schemaToForm", () => { ); expect(form.sections[0]!.fields[0]).toMatchObject({ initialValue: "hi" }); }); + + // #2015 (the TUI twin of #1928): an argument declared with Zod's `.nullish()` + // has no top-level `type`/`enum` — they sit on the surviving `anyOf` branch — + // so before normalization every one of these degraded to a plain text field. + describe("nullable unions", () => { + it("renders a select for an anyOf string-enum|null argument", () => { + const form = schemaToForm( + { + properties: { + direction: { + anyOf: [ + { type: "string", enum: ["envio", "recebimento"] }, + { type: "null" }, + ], + }, + }, + }, + "nullableEnum", + ); + expect(form.sections[0]!.fields[0]).toMatchObject({ + name: "direction", + type: "select", + options: [ + { label: "envio", value: "envio" }, + { label: "recebimento", value: "recebimento" }, + ], + }); + }); + + it("maps the remaining nullable scalar branches to their typed fields", () => { + const form = schemaToForm( + { + properties: { + reference: { anyOf: [{ type: "string" }, { type: "null" }] }, + quantity: { + anyOf: [ + { type: "integer", minimum: 1, maximum: 9 }, + { type: "null" }, + ], + }, + ratio: { anyOf: [{ type: "number" }, { type: "null" }] }, + express: { anyOf: [{ type: "boolean" }, { type: "null" }] }, + }, + }, + "nullableScalars", + ); + const fields = form.sections[0]!.fields; + expect(fields.map((f) => ({ name: f.name, type: f.type }))).toEqual([ + { name: "reference", type: "string" }, + { name: "quantity", type: "integer" }, + { name: "ratio", type: "float" }, + { name: "express", type: "boolean" }, + ]); + // The branch's own constraints are hoisted along with its type. + expect(fields[1]).toMatchObject({ min: 1, max: 9 }); + }); + + it("renders a select for a nullable array-of-enum argument", () => { + const form = schemaToForm( + { + properties: { + tags: { + anyOf: [ + { type: "array", items: { enum: ["a", "b"] } }, + { type: "null" }, + ], + }, + }, + }, + "nullableArrayEnum", + ); + expect(form.sections[0]!.fields[0]).toMatchObject({ + type: "select", + options: [ + { label: "a", value: "a" }, + { label: "b", value: "b" }, + ], + }); + }); + + // With the null left in the list, the all-strings check would reject the + // enum and this would degrade to a plain text field. + it("renders a select for a type: [string, null] enum, null stripped", () => { + const form = schemaToForm( + { + properties: { + direction: { + type: ["string", "null"], + enum: ["envio", "recebimento", null], + }, + }, + }, + "typeArrayNullableEnum", + ); + expect(form.sections[0]!.fields[0]).toMatchObject({ + type: "select", + options: [ + { label: "envio", value: "envio" }, + { label: "recebimento", value: "recebimento" }, + ], + }); + }); + + it("collapses the type: [T, null] encoding too", () => { + const form = schemaToForm( + { + properties: { + note: { type: ["string", "null"] }, + flag: { type: ["boolean", "null"] }, + }, + }, + "typeArrayNull", + ); + expect( + form.sections[0]!.fields.map((f) => ({ name: f.name, type: f.type })), + ).toEqual([ + { name: "note", type: "string" }, + { name: "flag", type: "boolean" }, + ]); + }); + + // ink-form hands a select's stringified value straight back, so a numeric + // enum routed to a select would submit "1" for 1. The typed field loses the + // enum constraint but keeps the value's type — the safer loss. + it("routes a nullable numeric enum to its typed field, not a select", () => { + const form = schemaToForm( + { + properties: { + level: { + anyOf: [{ type: "integer", enum: [1, 2] }, { type: "null" }], + }, + ratio: { + anyOf: [{ type: "number", enum: [0.5, 1.5] }, { type: "null" }], + }, + }, + }, + "numericEnum", + ); + expect( + form.sections[0]!.fields.map((f) => ({ name: f.name, type: f.type })), + ).toEqual([ + { name: "level", type: "integer" }, + { name: "ratio", type: "float" }, + ]); + }); + + it("routes a plain numeric enum to its typed field too", () => { + const form = schemaToForm( + { properties: { level: { type: "integer", enum: [1, 2] } } }, + "plainNumericEnum", + ); + expect(form.sections[0]!.fields[0]).toMatchObject({ type: "integer" }); + }); + + it("leaves a union of two real types as a plain string field", () => { + const form = schemaToForm( + { + properties: { + mixed: { anyOf: [{ type: "string" }, { type: "number" }] }, + }, + }, + "realUnion", + ); + expect(form.sections[0]!.fields[0]).toMatchObject({ + name: "mixed", + type: "string", + }); + }); + }); }); diff --git a/clients/tui/src/utils/schemaToForm.ts b/clients/tui/src/utils/schemaToForm.ts index bbb730f2..6332450c 100644 --- a/clients/tui/src/utils/schemaToForm.ts +++ b/clients/tui/src/utils/schemaToForm.ts @@ -3,10 +3,19 @@ */ import type { FormStructure, FormSection, FormField } from "ink-form"; +import { + isStringEnum, + normalizeNullableUnion, +} from "@inspector/core/json/nullableUnion.js"; /** Minimal JSON Schema property shape used when building tool parameter forms */ interface JsonSchemaProperty { - type?: string; + /** + * An array here is the `["string", "null"]` nullable encoding, which + * {@link normalizeNullableUnion} collapses to a single name before the + * dispatch below reads it. + */ + type?: string | string[]; title?: string; enum?: unknown[]; /** Non-standard legacy support: titles for enum values */ @@ -15,6 +24,8 @@ interface JsonSchemaProperty { minimum?: number; maximum?: number; default?: unknown; + /** Present on a nullable union; see {@link normalizeNullableUnion}. */ + anyOf?: readonly unknown[]; } /** @@ -68,9 +79,16 @@ export function schemaToForm( // `properties` values are `unknown` (the SDK schema admits anything), so // guard before treating a value as a schema object — a malformed server // schema with e.g. `properties: { foo: null }` must not throw on `.title`. - const property = ( - typeof prop === "object" && prop !== null ? prop : {} - ) as JsonSchemaProperty; + // Flatten a nullable union (`anyOf: [X, {type:"null"}]`, `type: [X,"null"]`) + // before dispatching. Every branch below reads a single `type` string, and + // for an enum the `enum` keyword sits on the union's surviving branch — so + // without this an argument declared with Zod's `.nullish()` loses its + // select and degrades to a plain text field (#2015, the TUI twin of #1928). + const property = normalizeNullableUnion( + (typeof prop === "object" && prop !== null + ? prop + : {}) as JsonSchemaProperty, + ); const baseField = { name: key, label: property.title || key, @@ -90,12 +108,17 @@ export function schemaToForm( ...baseField, options: toSelectOptions(property.items.enum, property.items.enumNames), } as FormField; - } else if (property.enum) { - // Single select + } else if (isStringEnum(property.enum)) { + // Single select. Gated on the members being strings because + // `toSelectOptions` stringifies them and ink-form hands the string + // straight back: a numeric `enum: [1, 2]` would submit `"1"` and violate + // the schema. A typed non-string enum falls through to its typed field + // below, which loses the enum constraint but keeps the value's type — + // the safer of the two losses. field = { type: "select", ...baseField, - options: toSelectOptions(property.enum, property.enumNames), + options: toSelectOptions(property.enum ?? [], property.enumNames), } as FormField; } else { // Map JSON Schema types to ink-form types diff --git a/clients/web/README.md b/clients/web/README.md index a3facd92..01873854 100644 --- a/clients/web/README.md +++ b/clients/web/README.md @@ -162,7 +162,9 @@ The dev/prod backend guards every `/api/*` route with `x-mcp-remote-auth: Bearer ## Host binding & the origin allow-list -Both the prod backend (`server/web-server-config.ts`) and the dev Vite server (`vite.config.ts`) resolve their bind host through one shared guard, `server/resolve-bind-host.ts`. It binds `localhost` by default and **refuses an all-interfaces host** (`0.0.0.0`, `::`, empty, or any equivalent spelling — `0`, `0x0`, `0.0`, `::0`, `::ffff:0.0.0.0`, … are all folded to the wildcard and refused) — which would expose the process-spawning backend to the whole network, the exposure DNS-rebinding attacks target — unless `DANGEROUSLY_BIND_ALL_INTERFACES=true` is set. The Docker image sets that flag (a container must bind `0.0.0.0` to be reachable through `-p`); a bare `HOST=0.0.0.0` anywhere else exits with an actionable error. +Both the prod backend (`server/web-server-config.ts`) and the dev Vite server (`vite.config.ts`) resolve their bind host through one shared guard, `server/resolve-bind-host.ts`. It binds **`127.0.0.1`** by default and **refuses an all-interfaces host** (`0.0.0.0`, `::`, empty, or any equivalent spelling — `0`, `0x0`, `0.0`, `::0`, `::ffff:0.0.0.0`, … are all folded to the wildcard and refused) — which would expose the process-spawning backend to the whole network, the exposure DNS-rebinding attacks target — unless `DANGEROUSLY_BIND_ALL_INTERFACES=true` is set. The Docker image sets that flag (a container must bind `0.0.0.0` to be reachable through `-p`); a bare `HOST=0.0.0.0` anywhere else exits with an actionable error. + +**The default is the IPv4 loopback _address_, deliberately not the name `localhost`** (#1951). `server.listen(port, host)` resolves a name through `dns.lookup` and binds the **single** address it gets back; since Node 17 the default result order is `verbatim`, so on a glibc Linux host — whose `/etc/hosts` maps `localhost` to both `127.0.0.1` and `::1` — that first address is `::1`. `HOST=localhost` there binds IPv6 loopback **only** and refuses every IPv4 client. On a desktop this is invisible (the browser resolves `localhost` the same way and lands on `::1` too); it breaks wherever the connection is made by something that pins `127.0.0.1` — a VS Code dev container's port forwarder, `ssh -L 6274:127.0.0.1:6274`, a container healthcheck. Binding the address takes the resolver out of the decision. Browsing to `http://localhost:PORT` still works, since browsers fall back across address families and the loopback allow-list covers all three forms. An explicit `HOST=localhost` is still honored as typed — the single-family bind is then your choice. The backend's `/api/*` routes also enforce an **origin allow-list** (`allowedOrigins`) as DNS-rebinding protection. When left to default on a loopback host, it expands to all three interchangeable loopback origin forms for the port — `http://localhost:PORT`, `http://127.0.0.1:PORT`, and `http://[::1]:PORT` — because `localhost` resolves to either IPv4 or IPv6 loopback and Node/Vite may bind the IPv6 form, so the browser can legitimately arrive at `http://[::1]:PORT`. Set `ALLOWED_ORIGINS` (comma-separated) to override; entries are canonicalized (`new URL(o).origin`), so a trailing slash / uppercase host / explicit `:80` still match. **Each entry must include the scheme** — `http://localhost:6274`, not `localhost:6274` (a scheme-less value is dropped with a warning). `ALLOWED_ORIGINS` **replaces** the default list (it does not merge), so **list every origin you'll browse from, including the loopback forms** you still want (`http://localhost:PORT`, `http://127.0.0.1:PORT`, `http://[::1]:PORT`) — otherwise local access stops working. A blank `ALLOWED_ORIGINS` does **not** disable the check — it falls back to the default (fail closed); there is no env knob to turn origin validation off. @@ -176,7 +178,7 @@ The guard blocks only the **wildcard** all-interfaces addresses. Binding a **spe The bind-host guard and the `ALLOWED_ORIGINS` allow-list apply to both the prod server and `--dev`. Note that in **`--dev`** the Vite dev server _additionally_ enforces its own `server.allowedHosts` Host-header check, whose default accepts loopback and IP-literal hosts. The host you **bind** is auto-allowed (Vite adds the resolved `server.host` — which this config sets from `HOST` — to the allow-list), so `HOST=` works out of the box under `--dev` too. What needs an explicit `server.allowedHosts` entry is reaching the dev server at a **different** name than the one bound — e.g. a wildcard bind reached by hostname, or a reverse-proxy domain. For those, prefer the prod server (`mcp-inspector --web`) or add the host to `server.allowedHosts`. -**MCP Apps caveats.** The MCP Apps sandbox runs on a **separate** port (`MCP_SANDBOX_PORT`, dynamic by default). For the Apps tab to work off loopback, that sandbox port must be independently reachable from the browser — pin it with `MCP_SANDBOX_PORT` and expose/forward it too (the Docker image `EXPOSE`s only `6274`). (Under a `0.0.0.0` wildcard bind the sandbox URL is advertised as `localhost`, which is reachable — a wildcard bind serves loopback — so only the port needs handling.) Also note the sandbox iframe is gated by a `frame-ancestors` CSP, and **a bracketed IPv6 literal is not a valid CSP host-source** — so MCP Apps requires browsing the app at a name or IPv4 (`localhost`, `127.0.0.1`, a hostname, a LAN IPv4), **not** a bare `http://[::1]:…` address. Finally, the sandbox URL is always `http://` — so **behind TLS** (an `https://` app page) the browser blocks the `http://…/sandbox` iframe as mixed content and MCP Apps can't render; the Apps tab needs a plain-`http` app origin today. +**MCP Apps caveats.** The MCP Apps sandbox runs on a **separate** port — `MCP_SANDBOX_PORT`, defaulting to a fixed **`6275`** (#2008; it was OS-assigned before, which meant it changed every run and so could never be named in a `forwardPorts` / `-p` / tunnel config written ahead of time). For the Apps tab to work off loopback, that sandbox port must be independently reachable from the browser — expose/forward `6275` alongside `6274`, or set `MCP_SANDBOX_PORT` to pick another. If the port is already taken the sandbox falls back to an OS-assigned one and warns, so a second Inspector still gets a working Apps tab locally — but the forwarded port is then wrong, which is what the warning tells you. (Under a `0.0.0.0` wildcard bind the sandbox URL is advertised as `localhost`, which is reachable — a wildcard bind serves loopback — so only the port needs handling.) Also note the sandbox iframe is gated by a `frame-ancestors` CSP, and **a bracketed IPv6 literal is not a valid CSP host-source** — so MCP Apps requires browsing the app at a name or IPv4 (`localhost`, `127.0.0.1`, a hostname, a LAN IPv4), **not** a bare `http://[::1]:…` address. Finally, the sandbox URL is always `http://` — so **behind TLS** (an `https://` app page) the browser blocks the `http://…/sandbox` iframe as mixed content and MCP Apps can't render; the Apps tab needs a plain-`http` app origin today. In every case, exposing the Inspector beyond loopback also means anyone who can reach it can drive its backend — keep authentication on (do **not** set `DANGEROUSLY_OMIT_AUTH`) and prefer a specific bind address over the wildcard. diff --git a/clients/web/server/resolve-bind-host.ts b/clients/web/server/resolve-bind-host.ts index a6d6fda1..f18945ce 100644 --- a/clients/web/server/resolve-bind-host.ts +++ b/clients/web/server/resolve-bind-host.ts @@ -26,21 +26,46 @@ function isEnabled(value: string | undefined): boolean { return v === "true" || v === "1"; } +/** + * The default bind host: the IPv4 loopback **address**, deliberately not the + * name `localhost`. + * + * `server.listen(port, host)` resolves a name through `dns.lookup` and binds the + * **single** address it gets back. Since Node 17 the default result order is + * `verbatim`, so Node takes whatever the resolver returns first rather than + * preferring IPv4 — and on a glibc Linux host with the stock `/etc/hosts` + * (`127.0.0.1 localhost` *and* `::1 localhost`) that is `::1`. `HOST=localhost` + * therefore binds IPv6 loopback **only**, and every IPv4 client is refused. + * + * That is invisible on a desktop, where the browser resolves `localhost` the + * same way and lands on `::1` too. It breaks wherever the connection is made by + * something that pins IPv4 `127.0.0.1` instead — a VS Code dev container's port + * forwarder (#1951), an `ssh -L …:127.0.0.1:…` tunnel, a container healthcheck. + * Binding the address directly removes the resolver from the decision, and the + * loopback origin allow-list already covers `localhost`, `127.0.0.1`, and + * `[::1]` alike, so browsing to `http://localhost:PORT` still works (browsers + * fall back across address families; a Node `listen` does not). + */ +export const DEFAULT_BIND_HOST = "127.0.0.1"; + /** * Resolve the bind hostname from `env` (default `process.env`), defaulting to - * `localhost`. Refuses an all-interfaces host (`0.0.0.0` / `::` / empty / their - * legacy spellings) unless {@link BIND_ALL_INTERFACES_ENV} is explicitly - * enabled — the published Docker image sets it, since a container must bind - * `0.0.0.0` to be reachable through `-p`. Throws (fail fast, loudly) rather than - * silently binding wide open. The returned value is trimmed and de-bracketed - * (an IPv6 literal is returned bare, e.g. `HOST=[::1]` → `::1`) so detection, - * `listen()`, and the origin list all consume the same value; `formatHostForUrl` - * re-adds the brackets wherever a URL is built. + * {@link DEFAULT_BIND_HOST}. Refuses an all-interfaces host (`0.0.0.0` / `::` / + * empty / their legacy spellings) unless {@link BIND_ALL_INTERFACES_ENV} is + * explicitly enabled — the published Docker image sets it, since a container + * must bind `0.0.0.0` to be reachable through `-p`. Throws (fail fast, loudly) + * rather than silently binding wide open. The returned value is trimmed and + * de-bracketed (an IPv6 literal is returned bare, e.g. `HOST=[::1]` → `::1`) so + * detection, `listen()`, and the origin list all consume the same value; + * `formatHostForUrl` re-adds the brackets wherever a URL is built. + * + * An explicit `HOST=localhost` is honored as typed — the single-family bind + * described on {@link DEFAULT_BIND_HOST} is then the caller's choice, not ours. */ export function resolveBindHostname( env: NodeJS.ProcessEnv = process.env, ): string { - const host = (env.HOST ?? "localhost").trim(); + const host = (env.HOST ?? DEFAULT_BIND_HOST).trim(); if (isAllInterfacesHost(host) && !isEnabled(env[BIND_ALL_INTERFACES_ENV])) { // Show the resolved address when it differs from the typed spelling — the // guard now catches forms the resolver folds to the wildcard (a fullwidth diff --git a/clients/web/server/sandbox-controller.ts b/clients/web/server/sandbox-controller.ts index e4b0d53e..50d4c440 100644 --- a/clients/web/server/sandbox-controller.ts +++ b/clients/web/server/sandbox-controller.ts @@ -11,13 +11,14 @@ import { canonicalUrlHost, isAllInterfacesHost, } from "../../../core/node/hostUrl.ts"; +import { DEFAULT_BIND_HOST } from "./resolve-bind-host.js"; const __dirname = dirname(fileURLToPath(import.meta.url)); export interface SandboxControllerOptions { /** Port to bind (0 = dynamic). */ port: number; - /** Host to bind (default localhost). */ + /** Host to bind (default {@link DEFAULT_BIND_HOST}). */ host?: string; /** * The backend's origin allow-list (the embedder origins). Used to build the @@ -95,10 +96,28 @@ function parseListenPort(raw: string | undefined): number | undefined { } /** - * Resolve sandbox port from env: MCP_SANDBOX_PORT → SERVER_PORT → 0 (dynamic). - * An invalid value falls through rather than crashing the boot; a set-but-invalid - * MCP_SANDBOX_PORT (the dedicated knob) is warned so the fall-through isn't - * silent — matching the warn-and-drop precedent for ALLOWED_ORIGINS. + * The default sandbox listen port — **fixed**, deliberately not `0`. + * + * A `0` default lets the OS assign, which is fine on a desktop (the browser + * learns the port from `/api/config`) but unusable anywhere the port has to be + * declared *before* the process starts: a dev container's `forwardPorts`, a + * `docker run -p`, a k8s service, an SSH tunnel. The port changed on every run, + * so it could never be named in that config and the Apps tab was unreachable + * (#2008). `CLIENT_PORT` has always defaulted to a fixed `6274` for the same + * reason; this is its sandbox counterpart. + * + * `6275` sits next to the web port so the pair is easy to forward together. It + * is not a v1 carry-over — v2 has no proxy server on it. + */ +export const DEFAULT_SANDBOX_PORT = 6275; + +/** + * Resolve sandbox port from env: MCP_SANDBOX_PORT → SERVER_PORT → + * {@link DEFAULT_SANDBOX_PORT}. An invalid value falls through rather than + * crashing the boot; a set-but-invalid MCP_SANDBOX_PORT (the dedicated knob) is + * warned so the fall-through isn't silent — matching the warn-and-drop + * precedent for ALLOWED_ORIGINS. An explicit `0` is still honored as "let the + * OS assign" ({@link parseListenPort} accepts it). */ export function resolveSandboxPort(): number { const fromSandbox = parseListenPort(process.env.MCP_SANDBOX_PORT); @@ -107,13 +126,21 @@ export function resolveSandboxPort(): number { `Ignoring invalid MCP_SANDBOX_PORT="${process.env.MCP_SANDBOX_PORT}" (need an integer 0–65535); falling back.`, ); } - return fromSandbox ?? parseListenPort(process.env.SERVER_PORT) ?? 0; + return ( + fromSandbox ?? + parseListenPort(process.env.SERVER_PORT) ?? + DEFAULT_SANDBOX_PORT + ); } export function createSandboxController( options: SandboxControllerOptions, ): SandboxController { - const { port, host = "localhost", allowedOrigins } = options; + // Defaulted to the same address the web server binds, never the name + // `localhost` — a name resolves to one address family and would reintroduce + // the #1951 split (web on IPv4, sandbox on IPv6) for any future call site + // that omits `host`. Both call sites pass `config.sandboxHost` today. + const { port, host = DEFAULT_BIND_HOST, allowedOrigins } = options; let server: Server | null = null; let sandboxUrl: string | null = null; @@ -177,8 +204,28 @@ export function createSandboxController( }); res.end(sandboxHtml); }); + // A taken port used to mean "no sandbox at all", which was tolerable + // only because the default was dynamic and therefore never collided. + // Now that it's fixed (see DEFAULT_SANDBOX_PORT), a second Inspector — + // or anything else already on 6275 — would take the Apps tab down, so + // retry once on an OS-assigned port. The retry is announced loudly: + // whoever pinned MCP_SANDBOX_PORT to forward it needs to know the pin + // didn't take, and a silently-moved port is exactly the failure this + // default was introduced to fix. + let retriedDynamic = false; server.on("error", (err: NodeJS.ErrnoException) => { if (err.code === "EADDRINUSE") { + if (!retriedDynamic && port !== 0) { + retriedDynamic = true; + console.warn( + `Sandbox: port ${port} in use; falling back to an OS-assigned port. ` + + `The MCP Apps tab will work locally, but the sandbox is no longer ` + + `on a predictable port — set MCP_SANDBOX_PORT to a free one if you ` + + `need to forward it (container / SSH tunnel / reverse proxy).`, + ); + server!.listen(0, host); + return; + } console.error( `Sandbox: port ${port || "dynamic"} in use. MCP Apps tab may not work.`, ); diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index 0869d0d5..8ca842ed 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -598,6 +598,7 @@ const DEFAULT_USE_INSPECTOR_CLIENT: ReturnType = { serverInfo: undefined, instructions: undefined, excludedTools: [], + malformedListItems: [], appRendererClient: null, connect: vi.fn().mockResolvedValue(undefined), disconnect: vi.fn().mockResolvedValue(undefined), diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 509fc079..72d8bd1f 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -63,6 +63,7 @@ import { ManagedRequestorTasksState } from "@inspector/core/mcp/state/managedReq import { ResourceSubscriptionsState } from "@inspector/core/mcp/state/resourceSubscriptionsState.js"; import { cleanRoots, + oauthAuthorizationParamsFromSettings, serializeMcpConfig, } from "@inspector/core/mcp/serverList.js"; import type { ClientConfig } from "@inspector/core/client/types.js"; @@ -163,6 +164,7 @@ import { import { buildExportFilename, downloadJsonFile } from "./lib/downloadFile"; import { INSPECTOR_SERVERS_TAB } from "./utils/inspectorTabs"; import { enrichProtocolEntries } from "./utils/correlateTransportErrors"; +import { visibleMalformedListItems } from "./utils/malformedListReport"; import { parseDeepLink, deepLinkConfigEquals, @@ -990,6 +992,7 @@ function App() { protocolEra, discoverResult, excludedTools, + malformedListItems, lastError, } = useInspectorClient(inspectorClient); const { @@ -1055,6 +1058,14 @@ function App() { setPaginatedListsOverride(null); }, [persistedPaginatedLists, activeServerId]); const paginatedLists = paginatedListsOverride ?? persistedPaginatedLists; + // The malformed-entry report is written by the aggregate walk's salvage. In + // paginated mode the tools/prompts/resources panels render the paged stores + // instead, which never write or clear it — so it would linger above a page it + // does not describe. See `visibleMalformedListItems` (#1909 + #1721). + const shownMalformedListItems = useMemo( + () => visibleMalformedListItems(malformedListItems, paginatedLists), + [malformedListItems, paginatedLists], + ); const connected = connectionStatus === "connected"; const toolsPagination = usePaginatedList({ connected, @@ -2301,11 +2312,15 @@ function App() { .map((m) => [m.key, m.value]), ) : undefined; + const serverAuthorizationParams = savedSettings + ? oauthAuthorizationParamsFromSettings(savedSettings) + : undefined; const oauthFromServer = savedSettings && (savedSettings.oauthClientId || savedSettings.oauthClientSecret || savedSettings.oauthScopes || + serverAuthorizationParams || savedSettings.enterpriseManaged) ? { ...(savedSettings.oauthClientId && { @@ -2317,6 +2332,9 @@ function App() { ...(savedSettings.oauthScopes && { scope: savedSettings.oauthScopes, }), + ...(serverAuthorizationParams && { + authorizationParams: serverAuthorizationParams, + }), ...(savedSettings.enterpriseManaged && { enterpriseManaged: true, }), @@ -4376,6 +4394,7 @@ function App() { latencyMs={latencyMs} tools={tools} excludedTools={excludedTools} + malformedListItems={shownMalformedListItems} prompts={prompts} resources={resources} resourceTemplates={resourceTemplates} diff --git a/clients/web/src/components/elements/CopyButton/CopyButton.test.tsx b/clients/web/src/components/elements/CopyButton/CopyButton.test.tsx index 9a4b462a..0b663cae 100644 --- a/clients/web/src/components/elements/CopyButton/CopyButton.test.tsx +++ b/clients/web/src/components/elements/CopyButton/CopyButton.test.tsx @@ -10,6 +10,17 @@ describe("CopyButton", () => { expect(screen.getByText("⎘")).toBeInTheDocument(); }); + it("names itself 'Copy' with no label, and qualifies the name when given one", () => { + const { unmount } = renderWithMantine(); + expect(screen.getByRole("button", { name: "Copy" })).toBeInTheDocument(); + unmount(); + + renderWithMantine(); + expect( + screen.getByRole("button", { name: "Copy ID Token" }), + ).toBeInTheDocument(); + }); + it("triggers a click without throwing when clipboard is unavailable", async () => { const user = userEvent.setup(); renderWithMantine(); diff --git a/clients/web/src/components/elements/CopyButton/CopyButton.tsx b/clients/web/src/components/elements/CopyButton/CopyButton.tsx index 7a86ca08..5b87719d 100644 --- a/clients/web/src/components/elements/CopyButton/CopyButton.tsx +++ b/clients/web/src/components/elements/CopyButton/CopyButton.tsx @@ -11,6 +11,15 @@ export interface CopyButtonProps { * rows (e.g. beside a Code block). Icon size is unchanged. */ flush?: boolean; + /** + * Names what is being copied, for the accessible label only ("Copy ID + * Token"). Set it when more than one CopyButton can appear in the same + * region, so button-by-button screen-reader navigation can tell them apart; + * a lone control needs no qualifier. The visible tooltip stays "Copy" either + * way, and the accessible name keeps "Copy"/"Copied" as its first word so it + * still contains the visible text (WCAG 2.5.3). + */ + label?: string; } const CopyActionIcon = ActionIcon.withProps({ @@ -18,7 +27,8 @@ const CopyActionIcon = ActionIcon.withProps({ fz: 24, }); -export function CopyButton({ value, flush = false }: CopyButtonProps) { +export function CopyButton({ value, flush = false, label }: CopyButtonProps) { + const qualifier = label ? ` ${label}` : ""; return ( {({ copied, copy }) => ( @@ -26,7 +36,7 @@ export function CopyButton({ value, flush = false }: CopyButtonProps) { {copied ? "\u2713" : "\u2398"} diff --git a/clients/web/src/components/elements/MalformedItemsWarning/MalformedItemsWarning.stories.tsx b/clients/web/src/components/elements/MalformedItemsWarning/MalformedItemsWarning.stories.tsx new file mode 100644 index 00000000..756e8667 --- /dev/null +++ b/clients/web/src/components/elements/MalformedItemsWarning/MalformedItemsWarning.stories.tsx @@ -0,0 +1,89 @@ +import type { Meta, StoryObj } from "@storybook/react-vite"; +import { MalformedItemsWarning } from "./MalformedItemsWarning"; + +const meta: Meta = { + title: "Elements/MalformedItemsWarning", + component: MalformedItemsWarning, +}; + +export default meta; +type Story = StoryObj; + +/** The failure this was built for: a PHP server whose empty `annotations` + * object reaches the wire as `[]` (#1909). */ +export const OneDroppedTemplate: Story = { + args: { + method: "resources/templates/list", + what: "resource templates", + items: [ + { + method: "resources/templates/list", + index: 1, + label: "array_annotations", + reason: "annotations: Invalid input: expected object, received array", + }, + ], + }, +}; + +export const SeveralDropped: Story = { + args: { + method: "tools/list", + what: "tools", + items: [ + { + method: "tools/list", + index: 0, + label: "get_weather", + reason: "inputSchema: Invalid input: expected object, received string", + }, + { + method: "tools/list", + index: 4, + label: "add", + reason: "name: Invalid input: expected string, received number", + }, + ], + }, +}; + +/** An entry too broken to carry a name is reported by its position. */ +export const UnlabeledEntry: Story = { + args: { + method: "prompts/list", + what: "prompts", + items: [ + { + method: "prompts/list", + index: 2, + reason: "Invalid input: expected object, received null", + }, + ], + }, +}; + +/** Entries belonging to another list method are ignored — the same set is + * passed to every panel, and each selects its own. */ +export const OtherMethodOnly: Story = { + args: { + method: "resources/templates/list", + what: "resource templates", + items: [ + { + method: "tools/list", + index: 0, + label: "get_weather", + reason: "inputSchema: Invalid input: expected object, received string", + }, + ], + }, +}; + +/** The resting state: a conforming server, nothing rendered. */ +export const Conforming: Story = { + args: { + method: "resources/templates/list", + what: "resource templates", + items: [], + }, +}; diff --git a/clients/web/src/components/elements/MalformedItemsWarning/MalformedItemsWarning.test.tsx b/clients/web/src/components/elements/MalformedItemsWarning/MalformedItemsWarning.test.tsx new file mode 100644 index 00000000..ed8c03f0 --- /dev/null +++ b/clients/web/src/components/elements/MalformedItemsWarning/MalformedItemsWarning.test.tsx @@ -0,0 +1,97 @@ +import { describe, it, expect } from "vitest"; +import { screen } from "@testing-library/react"; +import type { MalformedListItem } from "@inspector/core/mcp"; +import { renderWithMantine } from "../../../test/renderWithMantine"; +import { MalformedItemsWarning } from "./MalformedItemsWarning"; + +const TEMPLATE_ENTRY: MalformedListItem = { + method: "resources/templates/list", + index: 1, + label: "array_annotations", + reason: "annotations: Invalid input: expected object, received array", +}; + +const TOOL_ENTRY: MalformedListItem = { + method: "tools/list", + index: 0, + label: "get_weather", + reason: "inputSchema: Invalid input: expected object, received string", +}; + +describe("MalformedItemsWarning", () => { + it("names the dropped entry and why it was dropped", () => { + renderWithMantine( + , + ); + + expect(screen.getByText("1 malformed entry dropped")).toBeInTheDocument(); + expect(screen.getByText("array_annotations")).toBeInTheDocument(); + expect(screen.getByText(TEMPLATE_ENTRY.reason)).toBeInTheDocument(); + }); + + it("renders nothing when the server was conforming", () => { + renderWithMantine( + , + ); + expect(screen.queryByRole("alert")).not.toBeInTheDocument(); + }); + + it("ignores entries belonging to another list method", () => { + // Every panel receives the whole set and selects its own — a tools failure + // must not surface above the templates list. + renderWithMantine( + , + ); + expect(screen.queryByRole("alert")).not.toBeInTheDocument(); + expect(screen.queryByText("get_weather")).not.toBeInTheDocument(); + }); + + it("pluralizes the count and lists every entry", () => { + renderWithMantine( + , + ); + + expect(screen.getByText("2 malformed entries dropped")).toBeInTheDocument(); + expect(screen.getByText("get_weather")).toBeInTheDocument(); + expect(screen.getByText("add")).toBeInTheDocument(); + // The template entry belongs to the other panel. + expect(screen.queryByText("array_annotations")).not.toBeInTheDocument(); + }); + + it("falls back to the position when the entry had no usable name", () => { + renderWithMantine( + , + ); + expect(screen.getByText("entry 2")).toBeInTheDocument(); + }); +}); diff --git a/clients/web/src/components/elements/MalformedItemsWarning/MalformedItemsWarning.tsx b/clients/web/src/components/elements/MalformedItemsWarning/MalformedItemsWarning.tsx new file mode 100644 index 00000000..26bc5d5c --- /dev/null +++ b/clients/web/src/components/elements/MalformedItemsWarning/MalformedItemsWarning.tsx @@ -0,0 +1,99 @@ +import { Alert, Code, List, ScrollArea, Text } from "@mantine/core"; +import type { MalformedListItem } from "@inspector/core/mcp"; + +export interface MalformedItemsWarningProps { + /** + * Every malformed entry the client is currently reporting, across all list + * methods — pass the hook's set straight through; the component selects its + * own by {@link method}. + */ + items: MalformedListItem[]; + /** The list method this panel renders, e.g. `"resources/templates/list"`. */ + method: string; + /** What the entries are, for the alert title — e.g. "resource templates". */ + what: string; +} + +// Yellow, not red: unlike ListLoadError this is not a failure — the list DID +// load and is rendered below. It reports what the server sent that the +// Inspector could not accept. +const WarningAlert = Alert.withProps({ + color: "yellow", + variant: "light", +}); + +// The entry list stays compact; the reasons are the payload, so they get the +// monospace treatment while the labels stay in body type. +const EntryList = List.withProps({ + size: "xs", + spacing: 4, + withPadding: true, +}); + +const EntryReason = Code.withProps({ + variant: "wrapping", +}); + +// A server can be wrong about many entries at once; cap the height so a badly +// broken list can't push the list itself off the sidebar. +const EntryScroll = ScrollArea.withProps({ + mah: 180, + type: "auto", +}); + +const SummaryText = Text.withProps({ + size: "xs", + mb: 4, +}); + +// The entry's identity, emphasized against its monospaced reason. +const EntryLabel = Text.withProps({ + component: "span", + size: "xs", + fw: 600, +}); + +/** `label` when the entry had one, else its position in the response. */ +function describeEntry(item: MalformedListItem): string { + return item.label ?? `entry ${item.index}`; +} + +/** + * "Some entries were dropped" — the visible half of the #1909 salvage. + * + * The client keeps the valid entries of a list whose result failed schema + * validation, rather than losing the whole list to one bad entry. Dropping them + * silently would trade one lie for another, so what was dropped, and why, is + * reported here beside the list that rendered without them. + */ +export function MalformedItemsWarning({ + items, + method, + what, +}: MalformedItemsWarningProps) { + const mine = items.filter((item) => item.method === method); + if (mine.length === 0) return null; + + return ( + + + The server returned {what} that don't match the MCP schema. The + rest of the list is shown below. + + + + {mine.map((item) => ( + + {describeEntry(item)}{" "} + {item.reason} + + ))} + + + + ); +} diff --git a/clients/web/src/components/groups/AppDetailPanel/AppDetailPanel.test.tsx b/clients/web/src/components/groups/AppDetailPanel/AppDetailPanel.test.tsx index effbd5a2..ad14b1c3 100644 --- a/clients/web/src/components/groups/AppDetailPanel/AppDetailPanel.test.tsx +++ b/clients/web/src/components/groups/AppDetailPanel/AppDetailPanel.test.tsx @@ -173,4 +173,30 @@ describe("AppDetailPanel", () => { rerender(); expect(input().value).toBe(""); }); + + // #2020: `hasMissingRequiredFields` sees a field with no value, but text a + // field could not turn into a value looks identical to it — both report + // `undefined` — so an optional argument was dropped from the call with no + // gate. The form reports draft validity for this reason. + it("disables Open App while a field holds text it cannot send", async () => { + const user = userEvent.setup(); + const jsonFieldTool: Tool = { + name: "chart_app", + title: "Chart App", + inputSchema: { + type: "object", + properties: { series: { type: "array", title: "Series" } }, + }, + }; + renderWithMantine(); + const openApp = screen.getByRole("button", { name: /open app/i }); + expect(openApp).not.toBeDisabled(); + + const jsonInput = screen.getByLabelText(/Series/); + await user.type(jsonInput, "x"); + expect(openApp).toBeDisabled(); + + await user.clear(jsonInput); + expect(openApp).not.toBeDisabled(); + }); }); diff --git a/clients/web/src/components/groups/AppDetailPanel/AppDetailPanel.tsx b/clients/web/src/components/groups/AppDetailPanel/AppDetailPanel.tsx index 8d5a2135..8d75f431 100644 --- a/clients/web/src/components/groups/AppDetailPanel/AppDetailPanel.tsx +++ b/clients/web/src/components/groups/AppDetailPanel/AppDetailPanel.tsx @@ -1,4 +1,5 @@ import { Button, Divider, ScrollArea, Stack, Text } from "@mantine/core"; +import { useState } from "react"; import { MdPlayArrow } from "react-icons/md"; import type { Tool } from "@modelcontextprotocol/client"; import { SchemaForm } from "../SchemaForm/SchemaForm"; @@ -58,7 +59,12 @@ export function AppDetailPanel({ /* v8 ignore next -- unreachable: Tool.inputSchema is always an object */ const formSchema = toFormSchema(inputSchema) ?? {}; const hasErrors = hasMissingRequiredFields(formSchema, formValues); - const disabled = isOpening || hasErrors; + // A field holding text it could not turn into a value (unparseable JSON, an + // unrepresentable number) reports `undefined`, which is indistinguishable + // from empty once it reaches `formValues` — so the form reports it directly + // and an optional argument can no longer be dropped silently (#2020). + const [hasInvalidDraft, setHasInvalidDraft] = useState(false); + const disabled = isOpening || hasErrors || hasInvalidDraft; const hasFields = hasInputFields(tool); return ( @@ -82,6 +88,7 @@ export function AppDetailPanel({ // tool's name to drop another app's in-progress field text. See // SchemaFormProps.resetKey. resetKey={tool.name} + onValidityChange={setHasInvalidDraft} /> { + const canvas = within(canvasElement); + expect(canvas.getByText("ID Token")).toBeInTheDocument(); + // Only the ID token is a JWT here, so exactly one decode toggle is offered. + await userEvent.click( + canvas.getByRole("button", { name: "Decode JWT for ID Token" }), + ); + expect(canvas.getByText(/"sub": "user-42"/)).toBeInTheDocument(); + }, +}; diff --git a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx index 9f9a8ea4..f9f3d2fa 100644 --- a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx +++ b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx @@ -473,6 +473,95 @@ describe("ConnectionInfoContent", () => { expect(screen.queryByText("Auth URL")).not.toBeInTheDocument(); expect(screen.queryByText("Scopes")).not.toBeInTheDocument(); expect(screen.queryByText("Access Token")).not.toBeInTheDocument(); + expect(screen.queryByText("ID Token")).not.toBeInTheDocument(); + }); + + it("renders the ID Token row when the token set carries one", () => { + renderWithMantine( + , + ); + expect(screen.getByText("Access Token")).toBeInTheDocument(); + expect(screen.getByText("ID Token")).toBeInTheDocument(); + expect(screen.getByText("id-token-456")).toBeInTheDocument(); + }); + + it("gives the two token rows' controls distinct accessible names", () => { + // Both rows carry a copy control, and both tokens here are JWTs so both + // carry a decode toggle. Screen-reader button navigation has only the + // accessible name to go on, so the four must not collide (#2019 review). + const jwt = "eyJhbGciOiJub25lIn0.eyJzdWIiOiJ1c2VyIn0."; + renderWithMantine( + , + ); + + const names = screen + .getAllByRole("button") + .map((button) => button.getAttribute("aria-label") ?? button.textContent) + .filter((name): name is string => Boolean(name)); + expect(new Set(names).size).toBe(names.length); + + for (const name of [ + "Copy Access Token", + "Copy ID Token", + "Decode JWT for Access Token", + "Decode JWT for ID Token", + ]) { + expect(screen.getByRole("button", { name })).toBeInTheDocument(); + } + }); + + it("renders the ID Token row on its own when there is no access token", () => { + renderWithMantine( + , + ); + expect(screen.queryByText("Access Token")).not.toBeInTheDocument(); + expect(screen.getByText("ID Token")).toBeInTheDocument(); + }); + + it("omits the ID Token row when only an access token is present", () => { + renderWithMantine( + , + ); + expect(screen.getByText("Access Token")).toBeInTheDocument(); + expect(screen.queryByText("ID Token")).not.toBeInTheDocument(); }); it("does not render OAuth section when oauth prop is omitted", () => { diff --git a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.tsx b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.tsx index a7cd53f2..dde264b6 100644 --- a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.tsx +++ b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.tsx @@ -24,7 +24,7 @@ import { import { ContentViewer } from "../../elements/ContentViewer/ContentViewer"; import { EraBadge } from "../../elements/EraBadge/EraBadge"; import { isModernEra } from "../../elements/EraBadge/eraUtils"; -import { OAuthAccessTokenField } from "./OAuthAccessTokenField"; +import { OAuthTokenField } from "./OAuthTokenField"; export interface OAuthDetails { protocol: "standard" | "ema"; @@ -34,6 +34,13 @@ export interface OAuthDetails { authUrl?: string; scopes?: string[]; accessToken?: string; + /** + * OIDC `id_token` from the stored token set, when the authorization server + * returned one. Shown for inspection only — the MCP authorization spec is + * plain OAuth 2.1 and the Inspector never treats this as a credential + * (#2019). Absent when the token set carries none, so no empty row renders. + */ + idToken?: string; /** EMA only — install-level IdP session for legs 1–2. */ idpSession?: "none" | "logged_in" | "expired"; } @@ -370,20 +377,27 @@ export function ConnectionInfoContent({ {formatScopes(oauth.scopes)} )} - {oauth.accessToken ? ( - - ) : ( - onClearOAuth && ( - - - {CLEAR_OAUTH_STATE_AND_DISCONNECT_LABEL} - - - ) + )} + {/* Viewer only — an `id_token` the AS happened to return, decoded + on request. It carries no clear action: clearing OAuth state is + one action for the whole token set, owned by the access-token + row (or the standalone button below when there is none). */} + {oauth.idToken && ( + + )} + {!oauth.accessToken && onClearOAuth && ( + + + {CLEAR_OAUTH_STATE_AND_DISCONNECT_LABEL} + + )} diff --git a/clients/web/src/components/groups/ConnectionInfoContent/OAuthAccessTokenField.test.tsx b/clients/web/src/components/groups/ConnectionInfoContent/OAuthAccessTokenField.test.tsx deleted file mode 100644 index f1380f12..00000000 --- a/clients/web/src/components/groups/ConnectionInfoContent/OAuthAccessTokenField.test.tsx +++ /dev/null @@ -1,90 +0,0 @@ -import { describe, it, expect, vi } from "vitest"; -import userEvent from "@testing-library/user-event"; -import { renderWithMantine, screen } from "../../../test/renderWithMantine"; -import { CLEAR_OAUTH_STATE_AND_DISCONNECT_LABEL } from "./ConnectionInfoContent"; -import { OAuthAccessTokenField } from "./OAuthAccessTokenField"; - -const jwt = "eyJhbGciOiJub25lIn0.eyJzdWIiOiJ1c2VyIn0."; - -describe("OAuthAccessTokenField", () => { - it("renders token with copy control beside the content", () => { - renderWithMantine(); - expect(screen.getByText("Access Token")).toBeInTheDocument(); - expect(screen.getByText(/eyJhbGciOiJub25lIn0/)).toBeInTheDocument(); - expect(screen.getByText(/eyJzdWIiOiJ1c2VyIn0/)).toBeInTheDocument(); - expect(screen.getByRole("button", { name: "Copy" })).toBeInTheDocument(); - }); - - it("replaces raw token with decoded JSON and restores on toggle", async () => { - const user = userEvent.setup(); - renderWithMantine(); - - expect(screen.getByText(/eyJhbGciOiJub25lIn0/)).toBeInTheDocument(); - await user.click(screen.getByRole("button", { name: "Decode JWT" })); - expect(screen.getByText(/"sub": "user"/)).toBeInTheDocument(); - expect(screen.queryByText(/eyJhbGciOiJub25lIn0/)).not.toBeInTheDocument(); - - await user.click(screen.getByRole("button", { name: "Show token" })); - expect(screen.getByText(/eyJhbGciOiJub25lIn0/)).toBeInTheDocument(); - expect(screen.queryByText(/"sub": "user"/)).not.toBeInTheDocument(); - }); - - it("omits decode toggle for opaque tokens", () => { - renderWithMantine( - , - ); - expect( - screen.queryByRole("button", { name: "Decode JWT" }), - ).not.toBeInTheDocument(); - }); - - it("copies decoded JSON while decode view is shown", async () => { - const user = userEvent.setup(); - const writeText = vi.fn().mockResolvedValue(undefined); - Object.defineProperty(navigator, "clipboard", { - value: { writeText }, - configurable: true, - }); - - renderWithMantine(); - await user.click(screen.getByRole("button", { name: "Decode JWT" })); - await user.click(screen.getByRole("button", { name: "Copy" })); - - expect(writeText).toHaveBeenCalledWith( - expect.stringContaining('"sub": "user"'), - ); - expect(writeText).not.toHaveBeenCalledWith(jwt); - }); - - it("copies the raw token while token view is shown", async () => { - const user = userEvent.setup(); - const writeText = vi.fn().mockResolvedValue(undefined); - Object.defineProperty(navigator, "clipboard", { - value: { writeText }, - configurable: true, - }); - - renderWithMantine(); - await user.click(screen.getByRole("button", { name: "Copy" })); - - expect(writeText).toHaveBeenCalledWith(jwt); - }); - - it("renders clear action on the access token header row", async () => { - const user = userEvent.setup(); - const onClear = vi.fn(); - renderWithMantine( - , - ); - await user.click( - screen.getByRole("button", { - name: CLEAR_OAUTH_STATE_AND_DISCONNECT_LABEL, - }), - ); - expect(onClear).toHaveBeenCalledTimes(1); - }); -}); diff --git a/clients/web/src/components/groups/ConnectionInfoContent/OAuthTokenField.test.tsx b/clients/web/src/components/groups/ConnectionInfoContent/OAuthTokenField.test.tsx new file mode 100644 index 00000000..a46e6538 --- /dev/null +++ b/clients/web/src/components/groups/ConnectionInfoContent/OAuthTokenField.test.tsx @@ -0,0 +1,139 @@ +import { describe, it, expect, vi } from "vitest"; +import userEvent from "@testing-library/user-event"; +import { renderWithMantine, screen } from "../../../test/renderWithMantine"; +import { CLEAR_OAUTH_STATE_AND_DISCONNECT_LABEL } from "./ConnectionInfoContent"; +import { OAuthTokenField } from "./OAuthTokenField"; + +const jwt = "eyJhbGciOiJub25lIn0.eyJzdWIiOiJ1c2VyIn0."; + +describe("OAuthTokenField", () => { + it("renders token with copy control beside the content", () => { + renderWithMantine(); + expect(screen.getByText("Access Token")).toBeInTheDocument(); + expect(screen.getByText(/eyJhbGciOiJub25lIn0/)).toBeInTheDocument(); + expect(screen.getByText(/eyJzdWIiOiJ1c2VyIn0/)).toBeInTheDocument(); + expect( + screen.getByRole("button", { name: "Copy Access Token" }), + ).toBeInTheDocument(); + }); + + it("replaces raw token with decoded JSON and restores on toggle", async () => { + const user = userEvent.setup(); + renderWithMantine(); + + expect(screen.getByText(/eyJhbGciOiJub25lIn0/)).toBeInTheDocument(); + await user.click( + screen.getByRole("button", { name: "Decode JWT for Access Token" }), + ); + expect(screen.getByText(/"sub": "user"/)).toBeInTheDocument(); + expect(screen.queryByText(/eyJhbGciOiJub25lIn0/)).not.toBeInTheDocument(); + + await user.click( + screen.getByRole("button", { name: "Show token for Access Token" }), + ); + expect(screen.getByText(/eyJhbGciOiJub25lIn0/)).toBeInTheDocument(); + expect(screen.queryByText(/"sub": "user"/)).not.toBeInTheDocument(); + }); + + it("omits decode toggle for opaque tokens", () => { + renderWithMantine( + , + ); + expect( + screen.queryByRole("button", { name: "Decode JWT for Access Token" }), + ).not.toBeInTheDocument(); + }); + + it("copies decoded JSON while decode view is shown", async () => { + const user = userEvent.setup(); + const writeText = vi.fn().mockResolvedValue(undefined); + Object.defineProperty(navigator, "clipboard", { + value: { writeText }, + configurable: true, + }); + + renderWithMantine(); + await user.click( + screen.getByRole("button", { name: "Decode JWT for Access Token" }), + ); + await user.click(screen.getByRole("button", { name: "Copy Access Token" })); + + expect(writeText).toHaveBeenCalledWith( + expect.stringContaining('"sub": "user"'), + ); + expect(writeText).not.toHaveBeenCalledWith(jwt); + }); + + it("copies the raw token while token view is shown", async () => { + const user = userEvent.setup(); + const writeText = vi.fn().mockResolvedValue(undefined); + Object.defineProperty(navigator, "clipboard", { + value: { writeText }, + configurable: true, + }); + + renderWithMantine(); + await user.click(screen.getByRole("button", { name: "Copy Access Token" })); + + expect(writeText).toHaveBeenCalledWith(jwt); + }); + + it("renders clear action on the access token header row", async () => { + const user = userEvent.setup(); + const onClear = vi.fn(); + renderWithMantine( + , + ); + await user.click( + screen.getByRole("button", { + name: CLEAR_OAUTH_STATE_AND_DISCONNECT_LABEL, + }), + ); + expect(onClear).toHaveBeenCalledTimes(1); + }); + + it("labels the row from the label prop and decodes an ID token", async () => { + const user = userEvent.setup(); + // header {"alg":"none"} / payload {"sub":"user"} — same shape an AS returns + // for an `id_token`; the decode path is identical to the access token's. + renderWithMantine(); + + expect(screen.getByText("ID Token")).toBeInTheDocument(); + expect(screen.queryByText("Access Token")).not.toBeInTheDocument(); + + await user.click( + screen.getByRole("button", { name: "Decode JWT for ID Token" }), + ); + expect(screen.getByText(/"sub": "user"/)).toBeInTheDocument(); + }); + + it("qualifies the decode and copy accessible names with the row label", () => { + // Two rows can render simultaneously (Access Token + ID Token), so the + // controls must not share accessible names — see #2019 review. + renderWithMantine(); + + expect( + screen.getByRole("button", { name: "Decode JWT for ID Token" }), + ).toBeInTheDocument(); + expect( + screen.getByRole("button", { name: "Copy ID Token" }), + ).toBeInTheDocument(); + }); + + it("omits the clear action when no handler is given", () => { + renderWithMantine(); + expect( + screen.queryByRole("button", { + name: CLEAR_OAUTH_STATE_AND_DISCONNECT_LABEL, + }), + ).not.toBeInTheDocument(); + }); +}); diff --git a/clients/web/src/components/groups/ConnectionInfoContent/OAuthAccessTokenField.tsx b/clients/web/src/components/groups/ConnectionInfoContent/OAuthTokenField.tsx similarity index 70% rename from clients/web/src/components/groups/ConnectionInfoContent/OAuthAccessTokenField.tsx rename to clients/web/src/components/groups/ConnectionInfoContent/OAuthTokenField.tsx index 825a8507..abd5c4a9 100644 --- a/clients/web/src/components/groups/ConnectionInfoContent/OAuthAccessTokenField.tsx +++ b/clients/web/src/components/groups/ConnectionInfoContent/OAuthTokenField.tsx @@ -3,8 +3,18 @@ import { Button, Code, Flex, Stack, Text } from "@mantine/core"; import { decodeJwtPayload, isJwtFormat } from "@inspector/core/auth/ema/jwt.js"; import { CopyButton } from "../../elements/CopyButton/CopyButton"; -export interface OAuthAccessTokenFieldProps { - accessToken: string; +/** + * One labelled OAuth token row: the raw value, a copy control, and — when the + * value has the three-segment JWT shape — an in-place "Decode JWT" toggle. + * + * Shared by the Access Token and ID Token rows (#2019). The decode is + * display-only (`decodeJwtPayload` does no signature verification), so this is + * a viewer: neither token is treated as a credential here. + */ +export interface OAuthTokenFieldProps { + /** Row caption, e.g. "Access Token" / "ID Token". */ + label: string; + token: string; onClear?: () => void; clearLabel?: string; } @@ -64,16 +74,17 @@ function JwtTokenText({ token }: { token: string }) { ); } -export function OAuthAccessTokenField({ - accessToken, +export function OAuthTokenField({ + label, + token, onClear, clearLabel = "Clear", -}: OAuthAccessTokenFieldProps) { +}: OAuthTokenFieldProps) { const [showDecoded, setShowDecoded] = useState(false); - const isJwt = isJwtFormat(accessToken); + const isJwt = isJwtFormat(token); const jwtDecoded = useMemo( - () => (isJwt ? decodeJwtPayload(accessToken) : undefined), - [accessToken, isJwt], + () => (isJwt ? decodeJwtPayload(token) : undefined), + [token, isJwt], ); const decodedText = useMemo(() => { @@ -85,17 +96,18 @@ export function OAuthAccessTokenField({ ); }, [jwtDecoded]); - const copyValue = showDecoded && decodedText ? decodedText : accessToken; + const copyValue = showDecoded && decodedText ? decodedText : token; return ( - Access Token + {label} {jwtDecoded && ( setShowDecoded((open) => !open)} aria-pressed={showDecoded} + aria-label={`${showDecoded ? "Show token" : "Decode JWT"} for ${label}`} > {showDecoded ? "Show token" : "Decode JWT"} @@ -113,13 +125,13 @@ export function OAuthAccessTokenField({ {showDecoded && decodedText ? ( decodedText ) : isJwt ? ( - + ) : ( - accessToken + token )} - + ); diff --git a/clients/web/src/components/groups/ConnectionInfoContent/oauthDetailsFromConnectionState.test.ts b/clients/web/src/components/groups/ConnectionInfoContent/oauthDetailsFromConnectionState.test.ts index d6a89da6..d047ab23 100644 --- a/clients/web/src/components/groups/ConnectionInfoContent/oauthDetailsFromConnectionState.test.ts +++ b/clients/web/src/components/groups/ConnectionInfoContent/oauthDetailsFromConnectionState.test.ts @@ -34,6 +34,39 @@ describe("oauthDetailsFromConnectionState", () => { }); }); + it("surfaces an id_token when the token set carries one", () => { + const state: OAuthConnectionState = { + authorized: true, + protocol: "standard", + serverUrl: "https://mcp.example.com/mcp", + tokens: { + access_token: "tok", + token_type: "Bearer", + id_token: "header.payload.signature", + }, + }; + + expect(oauthDetailsFromConnectionState(state)).toEqual({ + protocol: "standard", + authorized: true, + accessToken: "tok", + idToken: "header.payload.signature", + }); + }); + + it("omits idToken when the token set carries none", () => { + const state: OAuthConnectionState = { + authorized: true, + protocol: "standard", + serverUrl: "https://mcp.example.com/mcp", + tokens: { access_token: "tok", token_type: "Bearer" }, + }; + + expect(oauthDetailsFromConnectionState(state)).not.toHaveProperty( + "idToken", + ); + }); + it("includes EMA idp session when present", () => { const state: OAuthConnectionState = { authorized: false, diff --git a/clients/web/src/components/groups/ConnectionInfoContent/oauthDetailsFromConnectionState.ts b/clients/web/src/components/groups/ConnectionInfoContent/oauthDetailsFromConnectionState.ts index 14c8847e..2e514633 100644 --- a/clients/web/src/components/groups/ConnectionInfoContent/oauthDetailsFromConnectionState.ts +++ b/clients/web/src/components/groups/ConnectionInfoContent/oauthDetailsFromConnectionState.ts @@ -22,6 +22,11 @@ export function oauthDetailsFromConnectionState( ...(state.tokens?.access_token && { accessToken: state.tokens.access_token, }), + // Surfaced for inspection when the AS returned one; omitted entirely + // otherwise so Connection Info renders no empty row (#2019). + ...(state.tokens?.id_token && { + idToken: state.tokens.id_token, + }), ...(state.protocol === "ema" && state.ema?.idpSession && { idpSession: state.ema.idpSession }), }; diff --git a/clients/web/src/components/groups/ElicitationFormPanel/ElicitationFormPanel.test.tsx b/clients/web/src/components/groups/ElicitationFormPanel/ElicitationFormPanel.test.tsx index d78e9171..87df919f 100644 --- a/clients/web/src/components/groups/ElicitationFormPanel/ElicitationFormPanel.test.tsx +++ b/clients/web/src/components/groups/ElicitationFormPanel/ElicitationFormPanel.test.tsx @@ -126,4 +126,32 @@ describe("ElicitationFormPanel", () => { await user.type(screen.getByLabelText("Host"), "l"); expect(onChange).toHaveBeenCalledWith({ host: "l" }); }); + + // #2020: text a field cannot turn into a value reports `undefined`, so it is + // indistinguishable from an empty field in `values` — Submit stayed enabled + // and the answer went out without the property. + it("disables Submit while a field holds text it cannot send", async () => { + const user = userEvent.setup(); + const numberRequest: ElicitRequestFormParams = { + message: "Please provide the port.", + requestedSchema: { + type: "object" as const, + properties: { port: { type: "number" as const, title: "Port number" } }, + }, + }; + renderWithMantine( + , + ); + const submit = screen.getByRole("button", { name: "Submit" }); + expect(submit).toBeEnabled(); + + // Past MAX_SAFE_INTEGER the field reports no value rather than a rounded + // one, so the answer would go out without the property. + const input = screen.getByLabelText(/Port number/); + await user.type(input, "90071992547409910"); + expect(submit).toBeDisabled(); + + await user.clear(input); + expect(submit).toBeEnabled(); + }); }); diff --git a/clients/web/src/components/groups/ElicitationFormPanel/ElicitationFormPanel.tsx b/clients/web/src/components/groups/ElicitationFormPanel/ElicitationFormPanel.tsx index 5ec4fddc..07802433 100644 --- a/clients/web/src/components/groups/ElicitationFormPanel/ElicitationFormPanel.tsx +++ b/clients/web/src/components/groups/ElicitationFormPanel/ElicitationFormPanel.tsx @@ -7,6 +7,7 @@ import { Stack, Text, } from "@mantine/core"; +import { useState } from "react"; import type { ElicitRequestFormParams } from "@modelcontextprotocol/client"; import type { PendingRequestOrigin } from "@inspector/core/mcp/types.js"; import { @@ -88,8 +89,13 @@ export function ElicitationFormPanel({ busy = false, }: ElicitationFormPanelProps) { const requestedSchema = request.requestedSchema as InspectorFormSchema; + // Text a field could not turn into a value never reaches `values`, so the + // form has to report it for Submit to see it at all (#2020). + const [hasInvalidDraft, setHasInvalidDraft] = useState(false); const submitDisabled = - busy || hasMissingRequiredFields(requestedSchema, values); + busy || + hasMissingRequiredFields(requestedSchema, values) || + hasInvalidDraft; return ( {formatQuoted(request.message)} @@ -101,6 +107,7 @@ export function ElicitationFormPanel({ values={values} onChange={onChange} disabled={busy} + onValidityChange={setHasInvalidDraft} /> {formatWarning(serverName)} diff --git a/clients/web/src/components/groups/InlineElicitationRequest/InlineElicitationRequest.test.tsx b/clients/web/src/components/groups/InlineElicitationRequest/InlineElicitationRequest.test.tsx index ea065596..b988e2b0 100644 --- a/clients/web/src/components/groups/InlineElicitationRequest/InlineElicitationRequest.test.tsx +++ b/clients/web/src/components/groups/InlineElicitationRequest/InlineElicitationRequest.test.tsx @@ -161,4 +161,70 @@ describe("InlineElicitationRequest", () => { ); expect(screen.getByText("Host")).toBeInTheDocument(); }); + + // #2020: this inline form had no submit gate at all — neither the missing + // required answer that `ElicitationFormPanel` already checked, nor text a + // field could not turn into a value (which never reaches `values`). + describe("submit gating (#2020)", () => { + const requiredRequest = { + message: "Please provide your name.", + requestedSchema: { + type: "object" as const, + properties: { name: { type: "string" as const, title: "Name" } }, + required: ["name"], + }, + } satisfies ElicitRequest["params"]; + + const numberRequest = { + message: "Please provide the port.", + requestedSchema: { + type: "object" as const, + properties: { port: { type: "number" as const, title: "Port number" } }, + }, + } satisfies ElicitRequest["params"]; + + it("disables Submit while a required answer is missing", () => { + renderWithMantine( + , + ); + expect(screen.getByRole("button", { name: "Submit" })).toBeDisabled(); + }); + + it("enables Submit once the required answer is given", () => { + renderWithMantine( + , + ); + expect(screen.getByRole("button", { name: "Submit" })).toBeEnabled(); + }); + + it("disables Submit while a field holds text it cannot send", async () => { + const user = userEvent.setup(); + renderWithMantine( + , + ); + const submit = screen.getByRole("button", { name: "Submit" }); + expect(submit).toBeEnabled(); + + // Past MAX_SAFE_INTEGER the field reports no value rather than a rounded + // one, so the answer would go out without the property. + const input = screen.getByLabelText(/Port number/); + await user.type(input, "90071992547409910"); + expect(submit).toBeDisabled(); + + await user.clear(input); + expect(submit).toBeEnabled(); + }); + }); }); diff --git a/clients/web/src/components/groups/InlineElicitationRequest/InlineElicitationRequest.tsx b/clients/web/src/components/groups/InlineElicitationRequest/InlineElicitationRequest.tsx index 14bc6fe8..7a85e6d2 100644 --- a/clients/web/src/components/groups/InlineElicitationRequest/InlineElicitationRequest.tsx +++ b/clients/web/src/components/groups/InlineElicitationRequest/InlineElicitationRequest.tsx @@ -13,7 +13,11 @@ import type { ElicitRequestFormParams, } from "@modelcontextprotocol/client"; import type { PendingRequestOrigin } from "@inspector/core/mcp/types.js"; -import type { InspectorFormSchema } from "../../../utils/jsonUtils"; +import { useState } from "react"; +import { + hasMissingRequiredFields, + type InspectorFormSchema, +} from "../../../utils/jsonUtils"; import { SchemaForm } from "../SchemaForm/SchemaForm"; import { MrtrOriginNote } from "../../elements/MrtrOriginNote/MrtrOriginNote"; @@ -86,6 +90,19 @@ export function InlineElicitationRequest({ onSubmit, onCancel, }: InlineElicitationRequestProps) { + // Same gate as `ElicitationFormPanel`, which this is the inline form of: a + // missing required answer, or a field holding text it could not turn into a + // value, must not be submittable — neither is visible in `values` (#2020). + const [hasInvalidDraft, setHasInvalidDraft] = useState(false); + const formValues = values ?? {}; + const submitDisabled = + isFormMode(request) && + (hasMissingRequiredFields( + request.requestedSchema as InspectorFormSchema, + formValues, + ) || + hasInvalidDraft); + return ( @@ -100,8 +117,9 @@ export function InlineElicitationRequest({ {isFormMode(request) && ( )} @@ -120,7 +138,7 @@ export function InlineElicitationRequest({ Cancel {isFormMode(request) && ( - )} diff --git a/clients/web/src/components/groups/PromptControls/PromptControls.tsx b/clients/web/src/components/groups/PromptControls/PromptControls.tsx index 2a2d5364..2fbb5633 100644 --- a/clients/web/src/components/groups/PromptControls/PromptControls.tsx +++ b/clients/web/src/components/groups/PromptControls/PromptControls.tsx @@ -3,6 +3,8 @@ import { ClearButton } from "../../elements/ClearButton/ClearButton"; import type { Prompt } from "@modelcontextprotocol/client"; import { ListChangedIndicator } from "../../elements/ListChangedIndicator/ListChangedIndicator"; import { ListLoadError } from "../../elements/ListLoadError/ListLoadError"; +import { MalformedItemsWarning } from "../../elements/MalformedItemsWarning/MalformedItemsWarning"; +import type { MalformedListItem } from "@inspector/core/mcp"; import { ListPaginationControls, type ListPaginationControlsProps, @@ -41,6 +43,12 @@ export interface PromptControlsProps { * A failed list load, surfaced above the list instead of leaving the panel * empty (which reads as "this server has none") (#1953). */ + /** + * Entries the client dropped from this list because they failed the MCP + * schema. Rendered as a warning above the list, which still shows the rest + * (#1909). + */ + malformedListItems?: MalformedListItem[]; loadError?: Error | null; /** Pagination controls (#1721). */ pagination: ListPaginationControlsProps; @@ -54,6 +62,7 @@ export function PromptControls({ searchText = "", listChanged, onRefreshList, + malformedListItems = [], loadError, pagination, onSearchChange, @@ -83,6 +92,11 @@ export function PromptControls({ /> + {filteredPrompts.map((prompt) => ( diff --git a/clients/web/src/components/groups/ResourceControls/ResourceControls.tsx b/clients/web/src/components/groups/ResourceControls/ResourceControls.tsx index adf5aa1a..6a319fb1 100644 --- a/clients/web/src/components/groups/ResourceControls/ResourceControls.tsx +++ b/clients/web/src/components/groups/ResourceControls/ResourceControls.tsx @@ -14,6 +14,8 @@ import { isModernEra } from "../../elements/EraBadge/eraUtils"; import { SubscriptionStreamBadge } from "../../elements/SubscriptionStreamBadge/SubscriptionStreamBadge"; import { ListChangedIndicator } from "../../elements/ListChangedIndicator/ListChangedIndicator"; import { ListLoadError } from "../../elements/ListLoadError/ListLoadError"; +import { MalformedItemsWarning } from "../../elements/MalformedItemsWarning/MalformedItemsWarning"; +import type { MalformedListItem } from "@inspector/core/mcp"; import { ListPaginationControls, type ListPaginationControlsProps, @@ -73,6 +75,12 @@ export interface ResourceControlsProps { * A failed list load, surfaced above the list instead of leaving the panel * empty (which reads as "this server has none") (#1953). */ + /** + * Entries the client dropped from `resources/list` or + * `resources/templates/list` because they failed the MCP schema. Rendered as + * a warning above the list, which still shows the rest (#1909). + */ + malformedListItems?: MalformedListItem[]; loadError?: Error | null; /** Pagination controls for the Resources list (#1721). */ pagination: ListPaginationControlsProps; @@ -117,6 +125,7 @@ export function ResourceControls({ openSections: controlledOpenSections, listChanged, onRefreshList, + malformedListItems = [], loadError, pagination, onSearchChange, @@ -248,6 +257,16 @@ export function ResourceControls({ what="resources" onRetry={onRefreshList} /> + + {/* Stays inline: Accordion is a compound, `multiple`-discriminated generic, so `.withProps({ multiple: true, ... })` loses its JSX call signature (same tooling limit as Box). */} diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx index 5b9f064a..b1d3182f 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx @@ -2,6 +2,7 @@ import { useState } from "react"; import { describe, it, expect, vi } from "vitest"; import userEvent from "@testing-library/user-event"; import type { InspectorFormSchema } from "../../../utils/jsonUtils"; +import { toFormSchema } from "../../../utils/jsonUtils"; import { fireEvent, renderWithMantine, @@ -737,7 +738,52 @@ describe("SchemaForm", () => { expect(lastCall.config).toEqual([1, 2]); }); - it("falls back to passing raw string to onChange when JSON is invalid in JsonInput", async () => { + // The JSON field used to store unparseable text back as the *value*, which + // the next render re-stringified — so each keystroke added a layer of + // escaping (`[` → `"["` → `"\"[\""`). That compounding escape is #1928's + // original symptom, and it lived here rather than in the dispatch. It matters + // more since #2007, whose fix deliberately routes object unions to this + // editor: a fallback nobody can type into is not a fallback. + it("shows an error while the JSON draft does not parse", async () => { + const user = userEvent.setup(); + const schema: InspectorFormSchema = { + type: "object", + properties: { + config: { type: "array", title: "Config" }, + }, + }; + function Harness() { + const [values, setValues] = useState>({}); + return ( + + ); + } + renderWithMantine(); + + const jsonInput = screen.getByLabelText(/Config/) as HTMLTextAreaElement; + await user.type(jsonInput, "[[1,"); + // Invalid text yields no value, so without this the field would submit as + // absent while the user is still looking at what they typed. + expect(screen.getByText(/Not valid JSON/)).toBeInTheDocument(); + + await user.type(jsonInput, "2]"); + expect(screen.queryByText(/Not valid JSON/)).not.toBeInTheDocument(); + }); + + it("shows no error for an empty optional field", () => { + const schema: InspectorFormSchema = { + type: "object", + properties: { + config: { type: "array", title: "Config" }, + }, + }; + renderWithMantine( + , + ); + expect(screen.queryByText(/Not valid JSON/)).not.toBeInTheDocument(); + }); + + it("reports no value, not raw text, while the JSON is mid-edit", async () => { const user = userEvent.setup(); const onChange = vi.fn(); const schema: InspectorFormSchema = { @@ -751,9 +797,59 @@ describe("SchemaForm", () => { ); const jsonInput = screen.getByLabelText(/Config/) as HTMLTextAreaElement; await user.type(jsonInput, "x"); - expect(onChange).toHaveBeenCalled(); - const lastCall = onChange.mock.calls[onChange.mock.calls.length - 1][0]; - expect(typeof lastCall.config).toBe("string"); + expect(onChange).toHaveBeenLastCalledWith({ config: undefined }); + }); + + it("lets an array literal be typed one character at a time", async () => { + const user = userEvent.setup(); + const schema: InspectorFormSchema = { + type: "object", + properties: { + config: { type: "array", title: "Config" }, + }, + }; + // Drive the real controlled loop: each onChange feeds straight back in as + // `values`, which is what turned the old handler's raw-text write into a + // compounding re-escape. + function Harness() { + const [values, setValues] = useState>({}); + return ( + + ); + } + renderWithMantine(); + + const jsonInput = screen.getByLabelText(/Config/) as HTMLTextAreaElement; + // `[` is a userEvent keyboard descriptor, so it is escaped as `[[`. + await user.type(jsonInput, '[[1,"a"]'); + + // The box shows exactly what was typed — no injected quotes or backslashes. + expect(jsonInput.value).toBe('[1,"a"]'); + }); + + it("keeps an in-progress draft visible instead of rewriting it", async () => { + const user = userEvent.setup(); + const schema: InspectorFormSchema = { + type: "object", + properties: { + config: { type: "array", title: "Config" }, + }, + }; + function Harness() { + const [values, setValues] = useState>({}); + return ( + + ); + } + renderWithMantine(); + + const jsonInput = screen.getByLabelText(/Config/) as HTMLTextAreaElement; + await user.type(jsonInput, "[[1,"); + // Unparseable so far, and it must survive the re-render untouched. + expect(jsonInput.value).toBe("[1,"); + + await user.type(jsonInput, "2]"); + expect(jsonInput.value).toBe("[1,2]"); }); it("uses default values when value is undefined", () => { @@ -811,3 +907,886 @@ describe("SchemaForm", () => { expect(container.firstChild).not.toBeNull(); }); }); + +// #1928: "optional AND explicitly nullable" (Zod's `.nullish()`) compiles to a +// nullable union rather than a plain type. Before the normalization step these +// matched no branch and fell through to the raw-JSON fallback, where every +// keystroke re-escaped the value. +describe("SchemaForm nullable unions", () => { + it("renders a Select for an anyOf string-enum|null field", async () => { + const user = userEvent.setup(); + const onChange = vi.fn(); + const schema: InspectorFormSchema = { + type: "object", + properties: { + type: { + title: "Type", + anyOf: [ + { type: "string", enum: ["envio", "recebimento"] }, + { type: "null" }, + ], + }, + }, + }; + renderWithMantine( + , + ); + await user.click(screen.getByRole("textbox", { name: "Type" })); + await user.click( + await screen.findByRole("option", { name: "envio", hidden: true }), + ); + expect(onChange).toHaveBeenCalledWith({ type: "envio" }); + }); + + it("clears a nullable enum back to null", async () => { + const user = userEvent.setup(); + const onChange = vi.fn(); + const schema: InspectorFormSchema = { + type: "object", + properties: { + type: { + title: "Type", + anyOf: [{ type: "string", enum: ["envio"] }, { type: "null" }], + }, + }, + }; + renderWithMantine( + , + ); + // Mantine marks its combobox clear button `aria-hidden` (it is mouse-only, + // `tabIndex={-1}`), so it is only reachable with `hidden: true`. + await user.click(screen.getByRole("button", { hidden: true })); + expect(onChange).toHaveBeenCalledWith({ type: null }); + }); + + it("offers no clear affordance on a non-nullable enum", () => { + const schema: InspectorFormSchema = { + type: "object", + properties: { + format: { type: "string", title: "Format", enum: ["json"] }, + }, + }; + renderWithMantine( + , + ); + expect( + screen.queryByRole("button", { hidden: true }), + ).not.toBeInTheDocument(); + }); + + // The other supported nullable encoding: keywords stay at the top level, so + // the null sentinel sits inside the enum list rather than on a branch. + it("renders a Select for a type: [string, null] enum, without a null option", async () => { + const user = userEvent.setup(); + const onChange = vi.fn(); + // Built through `toFormSchema`, the same narrowing boundary every + // production call site uses, rather than cast into `InspectorFormSchema`. + // A `null` member is valid JSON Schema but outside that type's `string[]` + // `enum`, and this fixture *is* wire data — so the honest way to introduce + // it is the wire→form narrow, not a double cast that erases the mismatch. + const schema = toFormSchema({ + type: "object", + properties: { + direction: { + title: "Direction", + type: ["string", "null"], + enum: ["envio", "recebimento", null], + }, + }, + }); + renderWithMantine( + , + ); + await user.click(screen.getByRole("textbox", { name: "Direction" })); + const options = await screen.findAllByRole("option", { hidden: true }); + expect(options.map((option) => option.textContent)).toEqual([ + "envio", + "recebimento", + ]); + await user.click(options[0]); + expect(onChange).toHaveBeenCalledWith({ direction: "envio" }); + }); + + it("renders a TextInput for a type: [string, null] field", async () => { + const user = userEvent.setup(); + const onChange = vi.fn(); + const schema: InspectorFormSchema = { + type: "object", + properties: { + note: { type: ["string", "null"], title: "Note" }, + }, + }; + renderWithMantine( + , + ); + await user.type(screen.getByLabelText(/Note/), "a"); + expect(onChange).toHaveBeenCalledWith({ note: "a" }); + }); + + it("renders a checkbox for an anyOf boolean|null field", async () => { + const user = userEvent.setup(); + const onChange = vi.fn(); + const schema: InspectorFormSchema = { + type: "object", + properties: { + enabled: { + title: "Enabled", + anyOf: [{ type: "boolean" }, { type: "null" }], + }, + }, + }; + renderWithMantine( + , + ); + await user.click(screen.getByLabelText("Enabled")); + expect(onChange).toHaveBeenCalledWith({ enabled: true }); + }); + + it("renders a MultiSelect for an anyOf array-of-enum|null field", async () => { + const user = userEvent.setup(); + const onChange = vi.fn(); + const schema: InspectorFormSchema = { + type: "object", + properties: { + tags: { + title: "Tags", + anyOf: [ + { type: "array", items: { type: "string", enum: ["a", "b"] } }, + { type: "null" }, + ], + }, + }, + }; + renderWithMantine( + , + ); + await user.click(screen.getByRole("textbox", { name: "Tags" })); + await user.click( + await screen.findByRole("option", { name: "a", hidden: true }), + ); + expect(onChange).toHaveBeenCalledWith({ tags: ["a"] }); + }); + + it("renders nested fields for an anyOf object|null field", async () => { + const user = userEvent.setup(); + const onChange = vi.fn(); + const schema: InspectorFormSchema = { + type: "object", + properties: { + profile: { + title: "Profile", + anyOf: [ + { + type: "object", + properties: { nick: { type: "string", title: "Nick" } }, + }, + { type: "null" }, + ], + }, + }, + }; + renderWithMantine( + , + ); + await user.type(screen.getByLabelText(/Nick/), "z"); + expect(onChange).toHaveBeenCalledWith({ profile: { nick: "z" } }); + }); + + // #2007: `z.array(z.union([z.object(…), z.object(…)]))` gives an `items.anyOf` + // whose branches carry no top-level `const`, so every MultiSelect option was + // the empty string — and Mantine *throws* on duplicate option values, greying + // out the whole tool panel. The nullable form below is reachable only because + // this PR now collapses it into `type: "array"`, so it has to be safe too. + it("falls back to the JSON input for an array of object-union items", () => { + const objectBranch = (name: string): InspectorFormSchema => ({ + type: "object", + properties: { type: { type: "string", const: name } }, + required: ["type"], + }); + const schema: InspectorFormSchema = { + type: "object", + properties: { + items: { + title: "Items", + type: "array", + items: { anyOf: [objectBranch("A"), objectBranch("B")] }, + }, + }, + }; + renderWithMantine( + , + ); + expect(screen.getByLabelText(/Items/).tagName).toBe("TEXTAREA"); + }); + + it("falls back to the JSON input for a nullable array of object-union items", () => { + const objectBranch = (name: string): InspectorFormSchema => ({ + type: "object", + properties: { type: { type: "string", const: name } }, + required: ["type"], + }); + const schema: InspectorFormSchema = { + type: "object", + properties: { + items: { + title: "Items", + anyOf: [ + { type: "array", items: { anyOf: [objectBranch("A")] } }, + { type: "null" }, + ], + }, + }, + }; + renderWithMantine( + , + ); + expect(screen.getByLabelText(/Items/).tagName).toBe("TEXTAREA"); + }); + + // Mantine's select is string-valued, so a numeric const would submit "1" for + // 1 — the same wrong-type-on-the-wire problem that keeps a numeric enum off + // the select path. + it("falls back to the JSON input for non-string anyOf consts", () => { + const schema: InspectorFormSchema = { + type: "object", + properties: { + items: { + title: "Items", + type: "array", + items: { anyOf: [{ const: 1 }, { const: 2 }] }, + }, + }, + }; + renderWithMantine( + , + ); + expect(screen.getByLabelText(/Items/).tagName).toBe("TEXTAREA"); + }); + + it("falls back to the JSON input when two anyOf branches share a const", () => { + const schema: InspectorFormSchema = { + type: "object", + properties: { + items: { + title: "Items", + type: "array", + items: { anyOf: [{ const: "dup" }, { const: "dup" }] }, + }, + }, + }; + renderWithMantine( + , + ); + expect(screen.getByLabelText(/Items/).tagName).toBe("TEXTAREA"); + }); + + it("falls back to a text input for a string oneOf with no consts", () => { + const schema: InspectorFormSchema = { + type: "object", + properties: { + mode: { + title: "Mode", + type: "string", + oneOf: [{ type: "string" }, { type: "string" }], + }, + }, + }; + renderWithMantine( + , + ); + expect(screen.getByLabelText(/Mode/).tagName).toBe("INPUT"); + }); + + it("still renders a MultiSelect when every anyOf branch has a distinct const", () => { + const schema: InspectorFormSchema = { + type: "object", + properties: { + items: { + title: "Items", + type: "array", + items: { anyOf: [{ const: "a", title: "Alpha" }, { const: "b" }] }, + }, + }, + }; + renderWithMantine( + , + ); + expect(screen.getByRole("textbox", { name: "Items" })).toBeInTheDocument(); + }); + + // Reachable only since the collapse landed: the nullable wrapper had no + // top-level `type` before, so it fell to the JSON editor rather than into a + // string-valued MultiSelect that would submit ["1"] for [1]. + it("falls back to the JSON input for a nullable array of numeric item enums", () => { + // Wire data again — a numeric `enum` is valid JSON Schema but outside + // `InspectorFormSchema`'s `string[]`, so it comes in through the narrow. + const schema = toFormSchema({ + type: "object", + properties: { + levels: { + title: "Levels", + anyOf: [{ type: "array", items: { enum: [1, 2] } }, { type: "null" }], + }, + }, + }); + renderWithMantine( + , + ); + expect(screen.getByLabelText(/Levels/).tagName).toBe("TEXTAREA"); + }); + + // The sibling enum rules null out even though the type list names it, so the + // field must not get a clear button that emits a value the schema rejects. + it("offers no clear button when a sibling enum excludes null", () => { + const schema = toFormSchema({ + type: "object", + properties: { + direction: { + title: "Direction", + type: ["string", "null"], + enum: ["envio", "recebimento"], + }, + }, + }); + renderWithMantine( + , + ); + expect( + screen.queryByRole("button", { hidden: true }), + ).not.toBeInTheDocument(); + }); + + it("still falls back to the JSON input for a union of two real types", () => { + const schema: InspectorFormSchema = { + type: "object", + properties: { + mixed: { + title: "Mixed", + anyOf: [{ type: "string" }, { type: "number" }], + }, + }, + }; + renderWithMantine( + , + ); + expect(screen.getByLabelText(/Mixed/).tagName).toBe("TEXTAREA"); + }); +}); + +// #2020: a field holding text it cannot turn into a value reports `undefined`, +// which is exactly what an *empty* field reports. That makes the two states +// indistinguishable to the caller, so invalid text in an optional field was +// submittable and simply arrived at the server absent. The form reports draft +// validity directly so a submit gate can see what `values` cannot. +describe("SchemaForm draft validity (#2020)", () => { + const jsonSchema: InspectorFormSchema = { + type: "object", + properties: { + config: { type: "array", title: "Config" }, + }, + }; + + function ValidityHarness({ + schema, + onValidityChange, + resetKey, + }: { + schema: InspectorFormSchema; + onValidityChange: (hasInvalidDraft: boolean) => void; + resetKey?: string; + }) { + const [values, setValues] = useState>({}); + return ( + + ); + } + + it("reports an empty optional field as valid", () => { + const onValidityChange = vi.fn(); + renderWithMantine( + , + ); + expect(onValidityChange).toHaveBeenLastCalledWith(false); + }); + + it("reports an unparseable JSON draft, then clears it once the text parses", async () => { + const user = userEvent.setup(); + const onValidityChange = vi.fn(); + renderWithMantine( + , + ); + + const jsonInput = screen.getByLabelText(/Config/) as HTMLTextAreaElement; + await user.type(jsonInput, "[[1,"); + expect(onValidityChange).toHaveBeenLastCalledWith(true); + + await user.type(jsonInput, "2]"); + expect(onValidityChange).toHaveBeenLastCalledWith(false); + }); + + it("reports a number this client cannot send exactly, and says so on the field", async () => { + const user = userEvent.setup(); + const onValidityChange = vi.fn(); + const schema: InspectorFormSchema = { + type: "object", + properties: { + divisor: { type: "number", title: "Divisor" }, + }, + }; + renderWithMantine( + , + ); + + // Past MAX_SAFE_INTEGER the field reports no value rather than a rounded + // one, so — like the JSON editor — the text on screen would otherwise be + // submitted as an absent argument. + const input = screen.getByLabelText(/Divisor/) as HTMLInputElement; + await user.type(input, "90071992547409910"); + expect(onValidityChange).toHaveBeenLastCalledWith(true); + expect(screen.getByText(/this field will be omitted/)).toBeInTheDocument(); + + await user.clear(input); + expect(onValidityChange).toHaveBeenLastCalledWith(false); + expect( + screen.queryByText(/this field will be omitted/), + ).not.toBeInTheDocument(); + }); + + it("stays invalid while any one field is invalid", async () => { + const user = userEvent.setup(); + const onValidityChange = vi.fn(); + const schema: InspectorFormSchema = { + type: "object", + properties: { + first: { type: "array", title: "First" }, + second: { type: "array", title: "Second" }, + }, + }; + renderWithMantine( + , + ); + + await user.type(screen.getByLabelText(/First/), "x"); + await user.type(screen.getByLabelText(/Second/), "y"); + expect(onValidityChange).toHaveBeenLastCalledWith(true); + + // One of the two recovering is not enough. + await user.clear(screen.getByLabelText(/First/)); + expect(onValidityChange).toHaveBeenLastCalledWith(true); + + await user.clear(screen.getByLabelText(/Second/)); + expect(onValidityChange).toHaveBeenLastCalledWith(false); + }); + + it("surfaces a nested object's invalid draft through the outer form", async () => { + const user = userEvent.setup(); + const onValidityChange = vi.fn(); + const schema: InspectorFormSchema = { + type: "object", + properties: { + outer: { + type: "object", + title: "Outer", + properties: { + config: { type: "array", title: "Config" }, + }, + }, + }, + }; + renderWithMantine( + , + ); + + await user.type(screen.getByLabelText(/Config/), "x"); + expect(onValidityChange).toHaveBeenLastCalledWith(true); + }); + + it("clears a stale invalid draft when the form moves to another entity", async () => { + const user = userEvent.setup(); + const onValidityChange = vi.fn(); + const { rerender } = renderWithMantine( + , + ); + + await user.type(screen.getByLabelText(/Config/), "x"); + expect(onValidityChange).toHaveBeenLastCalledWith(true); + + // Switching entities remounts the field, discarding its draft — so text + // typed for the previous one must not keep the new one blocked. + rerender( + , + ); + expect(onValidityChange).toHaveBeenLastCalledWith(false); + }); + + it("reports valid when the form unmounts holding an invalid draft", async () => { + const user = userEvent.setup(); + const onValidityChange = vi.fn(); + const { unmount } = renderWithMantine( + , + ); + + await user.type(screen.getByLabelText(/Config/), "x"); + expect(onValidityChange).toHaveBeenLastCalledWith(true); + + unmount(); + expect(onValidityChange).toHaveBeenLastCalledWith(false); + }); + + it("renders without a validity callback", async () => { + const user = userEvent.setup(); + renderWithMantine( + , + ); + await user.type(screen.getByLabelText(/Config/), "x"); + expect(screen.getByText(/Not valid JSON/)).toBeInTheDocument(); + }); +}); + +// Three separate reports of one defect: the JSON editor re-escaping the text +// being typed. #1853 saw it while typing an array, #1856 on the Backspace that +// first makes a valid array invalid, and #1885 on a nullable parameter whose +// default `null` had to be edited in place. All three come from the same +// mechanism — unparseable draft text stored back as the field's *value*, which +// the next controlled render re-`JSON.stringify`d, adding a layer of quotes and +// backslashes per keystroke. +// +// The mechanism was removed by the draft/value split in `SchemaJsonField` +// (#1928/#2007) and the nullable-union collapse that keeps a `T | null` field +// off this editor entirely (#1928). These lock each report's own reproduction +// to it, since the fixes were made for differently-framed issues and nothing +// otherwise pins the reported flows. +describe("JSON editor escaping (#1853, #1856, #1885)", () => { + function EscapingHarness({ + schema, + initial = {}, + }: { + schema: InspectorFormSchema; + initial?: Record; + }) { + const [values, setValues] = useState>(initial); + return ; + } + + // #1853: `batch_process_items`, typed character by character. + it("types a string array through without escaping it (#1853)", async () => { + const user = userEvent.setup(); + const schema: InspectorFormSchema = { + type: "object", + properties: { + itemIds: { + type: "array", + title: "Item Ids", + items: { type: "string" }, + }, + }, + required: ["itemIds"], + }; + renderWithMantine(); + + const jsonInput = screen.getByLabelText(/Item Ids/) as HTMLTextAreaElement; + // `[` opens a userEvent keyboard descriptor, so it is escaped as `[[`. + await user.type(jsonInput, '[["item-1","item-2"]'); + + expect(jsonInput.value).toBe('["item-1","item-2"]'); + expect(jsonInput.value).not.toContain("\\"); + }); + + // #1853 again, from the comment thread: the caret sitting *outside* the + // quotes of a `""` value. One keystroke there made the draft invalid, which + // is all it took — `""` + `a` rendered as `"\"\"a"`. + it("keeps a keystroke typed outside a string's quotes literal (#1853)", async () => { + const user = userEvent.setup(); + const schema: InspectorFormSchema = { + type: "object", + // No `type`, so the field lands on the JSON editor holding a string. + properties: { note: { title: "Note" } }, + }; + renderWithMantine( + , + ); + + const jsonInput = screen.getByLabelText(/Note/) as HTMLTextAreaElement; + await user.click(jsonInput); + await user.keyboard("{End}a"); + + expect(jsonInput.value).toBe('""a'); + }); + + // #1856: `sum_numbers`, Backspace with the caret after the closing `]`. + it("keeps the draft raw when Backspace invalidates an array (#1856)", async () => { + const user = userEvent.setup(); + const schema: InspectorFormSchema = { + type: "object", + properties: { + numbers: { + type: "array", + title: "Numbers", + items: { type: "number" }, + }, + }, + required: ["numbers"], + }; + renderWithMantine( + , + ); + + const jsonInput = screen.getByLabelText(/Numbers/) as HTMLTextAreaElement; + await user.click(jsonInput); + await user.keyboard("{End}{Backspace}"); + + // Exactly the seeded text minus its last character — the draft the reporter + // expected, where the field instead showed a quoted, escaped string. + expect(jsonInput.value).toBe("[\n 1,\n 2\n"); + + // Each further edit compounded the escaping, so keep going. + await user.keyboard("{Backspace}{Backspace}"); + expect(jsonInput.value).not.toContain("\\"); + expect(jsonInput.value.startsWith('"')).toBe(false); + + // And the draft is still live: closing it back up produces a real array. + await user.keyboard("2]"); + expect(jsonInput.value).toBe("[\n 1,\n 2]"); + }); + + // #1885: FastMCP's `b: int | None = None`, which compiles to an `anyOf` with + // a null branch and `default: null`. The reporter was editing the literal + // `null` token in a raw JSON box; collapsing the union routes the field to a + // real number input, so there is no JSON token to edit in the first place. + it("edits a nullable integer as a number input, not a null token (#1885)", async () => { + const user = userEvent.setup(); + const schema: InspectorFormSchema = { + type: "object", + properties: { + a: { type: "integer", title: "A" }, + b: { + title: "B", + anyOf: [{ type: "integer" }, { type: "null" }], + default: null, + }, + }, + required: ["a"], + }; + // No seeded value: the `null` has to come from the schema's `default`, + // which is where the reporter's did. + renderWithMantine(); + + const b = screen.getByLabelText(/^B$/) as HTMLInputElement; + expect(b.tagName).toBe("INPUT"); + // `null` is rendered as "no value", not as four editable characters. + expect(b.value).toBe(""); + + await user.click(b); + await user.keyboard("42{Backspace}"); + expect(b.value).toBe("4"); + expect(b.value).not.toContain("\\"); + }); + + // The same nullable-with-`default: null` shape on a field the collapse still + // sends to the JSON editor (an array union). Backspacing the `null` token + // there is the exact keystroke sequence from #1885's recording. + it("backspaces a null default in the JSON editor without escaping (#1885)", async () => { + const user = userEvent.setup(); + const schema: InspectorFormSchema = { + type: "object", + properties: { + cfg: { + title: "Cfg", + anyOf: [{ type: "array" }, { type: "null" }], + default: null, + }, + }, + }; + // Again unseeded, so the editor's opening `null` is the schema default + // being resolved — the state the reporter's recording starts from. + renderWithMantine(); + + const jsonInput = screen.getByLabelText(/Cfg/) as HTMLTextAreaElement; + expect(jsonInput.value).toBe("null"); + + await user.click(jsonInput); + await user.keyboard("{End}{Backspace}"); + expect(jsonInput.value).toBe("nul"); + + await user.keyboard("{Backspace}{Backspace}{Backspace}"); + expect(jsonInput.value).toBe(""); + expect(jsonInput.value).not.toContain("\\"); + }); +}); + +// A schema `default` is what a field *opens* with, not a value re-imposed on +// every unsendable draft. Unsendable text reports `undefined` by design, and +// `resolveValue` used to turn that straight back into the default — a change +// from the previous value, so the draft/value re-sync rewrote the box and +// reverted the keystroke. +// +// The two halves differed in how reachable they were, which the tests below +// mirror deliberately: +// +// - **The number field reverted in the app.** Numbers compare by value, so +// clearing a defaulted box (value `3` → resolved default `30`) always fired +// the re-sync and refilled itself. +// - **The JSON field was latent.** `collectSchemaDefaults` assigns +// `fieldSchema.default` itself, so a field seeded from the schema holds the +// *same reference* the substitution returns and nothing fires. It needs a +// structurally-equal but distinct value object to become observable — which +// is what parsed wire/deep-link values and rebuilt nested-object defaults +// produce, and what these tests construct. +describe("SchemaForm defaulted fields (#2026)", () => { + function DefaultHarness({ + schema, + initial = {}, + }: { + schema: InspectorFormSchema; + initial?: Record; + }) { + const [values, setValues] = useState>(initial); + return ; + } + + const arrayWithDefault: InspectorFormSchema = { + type: "object", + properties: { tags: { type: "array", title: "Tags", default: ["a"] } }, + }; + + it("opens the JSON editor on the schema default", () => { + renderWithMantine(); + expect((screen.getByLabelText(/Tags/) as HTMLTextAreaElement).value).toBe( + '[\n "a"\n]', + ); + }); + + it("keeps a keystroke typed into a defaulted JSON field", async () => { + const user = userEvent.setup(); + // Seeded with a value that is equal to the schema default but is not the + // same object — how values parsed off the wire or out of a deep link + // arrive. Sharing the reference (what `collectSchemaDefaults` produces) is + // what keeps this latent rather than what makes it safe. + renderWithMantine( + , + ); + + const jsonInput = screen.getByLabelText(/Tags/) as HTMLTextAreaElement; + await user.click(jsonInput); + await user.keyboard("{End}x"); + + // The invalid draft is the user's, not the default reasserting itself. + expect(jsonInput.value).toBe('[\n "a"\n]x'); + }); + + it("lets a defaulted JSON field be edited to a new value", async () => { + const user = userEvent.setup(); + // Distinct-object seed again, for the reason above. + renderWithMantine( + , + ); + + const jsonInput = screen.getByLabelText(/Tags/) as HTMLTextAreaElement; + await user.clear(jsonInput); + await user.type(jsonInput, '[["b"]'); + + expect(jsonInput.value).toBe('["b"]'); + expect(screen.queryByText(/Not valid JSON/)).not.toBeInTheDocument(); + }); + + it("lets a defaulted number field be emptied", async () => { + const user = userEvent.setup(); + const schema: InspectorFormSchema = { + type: "object", + properties: { n: { type: "integer", title: "N", default: 30 } }, + }; + renderWithMantine(); + + const n = screen.getByLabelText(/N/) as HTMLInputElement; + expect(n.value).toBe("30"); + + await user.click(n); + await user.keyboard("{End}{Backspace}{Backspace}"); + + // The box stays empty instead of refilling itself with the default. + expect(n.value).toBe(""); + }); + + // An explicit `null` is a value, not an absent one, so a non-null default + // must not be substituted for it. Note the field itself never emits `null` — + // clearing it reports `undefined` (pinned by "passes undefined to onChange + // when a number field is cleared") — so this arrives from parent state: a + // value received from the server, restored from a deep link, or written by a + // caller for a nullable schema. + it("shows an explicit null as empty, not as the default", () => { + const schema: InspectorFormSchema = { + type: "object", + properties: { + n: { + title: "N", + anyOf: [{ type: "integer" }, { type: "null" }], + default: 30, + }, + }, + }; + renderWithMantine(); + + expect((screen.getByLabelText(/^N$/) as HTMLInputElement).value).toBe(""); + }); + + it("still re-syncs a defaulted field when the value changes externally", async () => { + function ExternalHarness() { + const [values, setValues] = useState>({ + tags: ["a"], + }); + return ( + <> + + + + ); + } + const user = userEvent.setup(); + renderWithMantine(); + + const jsonInput = screen.getByLabelText(/Tags/) as HTMLTextAreaElement; + await user.click(screen.getByRole("button", { name: "load example" })); + expect(jsonInput.value).toBe('[\n "z"\n]'); + }); +}); diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx index 232b5327..152fe40f 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx @@ -8,10 +8,17 @@ import { Text, TextInput, } from "@mantine/core"; -import { useState } from "react"; +import { useCallback, useEffect, useRef, useState } from "react"; import { ClearButton } from "../../elements/ClearButton/ClearButton"; import { useValueChange } from "../../../hooks/useValueChange"; -import type { InspectorFormSchema } from "../../../utils/jsonUtils"; +import type { + InspectorFormSchema, + JsonSchemaConst, +} from "../../../utils/jsonUtils"; +import { + isStringEnum, + normalizeNullableUnion, +} from "@inspector/core/json/nullableUnion.js"; const FieldLabel = Text.withProps({ fw: 500, @@ -51,6 +58,58 @@ function toEnumData( return values; } +/** + * Build `Select`/`MultiSelect` option data from a list of `oneOf`/`anyOf` + * branches that are expected to be constants. + * + * Returns `null` when the branches are not usable as options, which sends the + * field to the JSON fallback instead. Four ways that happens: + * + * - **A non-string `const`.** Mantine's select is string-valued, so + * `anyOf: [{ const: 1 }, { const: 2 }]` would submit `["1"]` where the + * schema says `[1]` — the same wrong-type-on-the-wire problem that keeps a + * numeric `enum` off the select path. An inspector must not misreport what + * it sends, so this stays on the JSON editor, where the value keeps its type. + * - **No `const` at all.** An `anyOf` of *object* schemas — what + * `z.array(z.union([z.object(…), z.object(…)]))` compiles to — has no + * top-level `const` on any branch, so every option would be the empty + * string. Mantine **throws** on duplicate option values, which greys out the + * whole tool panel rather than degrading (#2007). + * - **Duplicate values.** Two branches sharing a `const` throw the same way. + * - **An empty option value**, which Mantine cannot render as selectable. + * + * A union of object shapes has no faithful dropdown anyway, so the JSON editor + * is the honest widget for it rather than a lossy or crashing one. + */ +function toConstOptions( + branches: (InspectorFormSchema | JsonSchemaConst)[], +): { value: string; label: string }[] | null { + const options: { value: string; label: string }[] = []; + const seen = new Set(); + for (const branch of branches) { + if (typeof branch.const !== "string") { + return null; + } + const value = branch.const; + if (value === "" || seen.has(value)) { + return null; + } + seen.add(value); + options.push({ value, label: branch.title ?? value }); + } + return options.length > 0 ? options : null; +} + +/** + * Whether an enum `Select` should offer a clear affordance. Only a schema that + * admits `null` does: clearing emits `null`, and an enum without a null branch + * would reject it. Without this a nullable enum is a one-way door — once a + * value is picked there is no way back to "no answer". + */ +function isClearable(fieldSchema: InspectorFormSchema): boolean { + return fieldSchema.nullable === true; +} + /** * Interpret whatever Mantine's `NumberInput` reported as the JSON value for the * field. Anything that is not a finite number becomes `undefined`, which is how @@ -90,16 +149,210 @@ function toNumericValue(raw: string | number): number | undefined { return Number.isSafeInteger(Math.trunc(parsed)) ? parsed : undefined; } -interface SchemaNumberInputProps { +/** Parse editor text, reporting `undefined` for anything that is not JSON yet. */ +function parseJsonDraft(text: string): unknown { + if (text.trim() === "") { + return undefined; + } + try { + return JSON.parse(text); + } catch { + return undefined; + } +} + +/** Structural equality for the draft/value re-sync, via canonical JSON. */ +function isSameJson(a: unknown, b: unknown): boolean { + if (a === undefined || b === undefined) { + return a === b; + } + return serializeJson(a) === serializeJson(b); +} + +/** + * How a field tells the enclosing `SchemaForm` whether the text it is holding + * can be sent. Keyed by field name rather than closed over per field so the + * reporter is referentially stable, which is what keeps the effect below from + * re-subscribing on every render. + */ +type DraftValidityReporter = (fieldName: string, isValid: boolean) => void; + +/** + * Publish a field's draft validity to the enclosing `SchemaForm`. + * + * Reported from an effect rather than from the change handler because validity + * also moves when the *parent* replaces the value — a cleared form or a loaded + * example re-syncs the draft and makes invalid text valid again without the + * user touching the field. Reporting during that render instead would mean + * updating another component mid-render, which React rejects. + * + * The cleanup reports the field as valid again, so a field that goes away — + * unmounted with the panel, or remounted under a new `resetKey` when the form + * moves to another entity — cannot leave a stale draft blocking submission. + */ +function useDraftValidity( + fieldName: string, + isValid: boolean, + onValidityChange: DraftValidityReporter, +): void { + useEffect(() => { + onValidityChange(fieldName, isValid); + return () => onValidityChange(fieldName, true); + }, [fieldName, isValid, onValidityChange]); +} + +interface SchemaJsonFieldProps { + fieldName: string; label: string; description?: string; withAsterisk: boolean; disabled: boolean; - value: number | undefined; + /** + * The field's own entry in the form's values — **not** run through + * `resolveValue`. A draft-holding field is handed the raw value and the + * schema default separately; see `defaultValue` for why. + */ + value: unknown; + /** The schema `default`, used to seed the draft only. See `defaultValue`. */ + defaultValue: unknown; + onChange: (value: unknown) => void; + onValidityChange: DraftValidityReporter; +} + +/** + * A `JsonInput` that keeps the text the user is typing, not just the value it + * currently parses to. + * + * This field used to drive the box straight off the parent's value and, when + * the text failed `JSON.parse`, store the **raw text** back as the value — which + * the next render re-`JSON.stringify`d, adding a layer of escaping per + * keystroke. Typing `[` gave `"["`, then `"\"[\""`, and the field was unusable + * within a few characters. That compounding escape is the original #1928 + * symptom, and it lived in this fallback rather than in the dispatch. + * + * Routing nullable fields to real widgets removed the common way of *landing* + * here, but it did not fix the editor — and #2007's fix deliberately sends a + * union of object shapes to it, so the editor itself has to be typeable. Hence + * the same split `SchemaNumberInput` uses: the raw text is the source of truth + * for what is *displayed*, while the parent only ever sees parsed JSON. + * + * While the text is mid-edit and does not parse, the parent is told + * `undefined` rather than handed the text. An inspector must not report a value + * it cannot send, and `undefined` is how "no value supplied" is spelled + * everywhere else in this form. + * + * That leaves invalid text and an empty field indistinguishable *to the value*, + * so the field reports the difference two other ways: it says so on itself via + * `error`, and it reports draft validity up through `onValidityChange` so the + * enclosing form's caller can disable Execute/Open App/Submit (#2020). Without + * the second channel an **optional** field's invalid text is simply dropped + * from a submission the user was allowed to make. + */ +function SchemaJsonField({ + fieldName, + value, + defaultValue, + onChange, + onValidityChange, + ...inputProps +}: SchemaJsonFieldProps) { + const [draft, setDraft] = useState(() => { + const initial = value === undefined ? defaultValue : value; + return initial === undefined ? "" : serializeJson(initial); + }); + + // Re-sync only when the parent's value genuinely diverges from what the draft + // parses to, which leaves an external reset (a cleared form, a loaded + // example) working while an in-progress `[` — whose parse is `undefined`, + // matching the `undefined` we just emitted — is left alone. + // + // The value compared here is the field's *raw* entry, never `resolveValue`'s + // default substitution (#2026). Unsendable text reports `undefined`, and a + // defaulted field resolves that straight back to its default — a change from + // the previous value, so comparing against the resolved one would rewrite the + // draft to the default and revert the keystroke. Almost every edit passes + // through an unsendable state, so a defaulted array or object argument would + // be uneditable. + // + // Would be, not was: this is latent under today's seeding, where + // `collectSchemaDefaults` assigns `fieldSchema.default` itself, so the + // substitution returns the *same reference* and `useValueChange` never fires. + // It goes live as soon as the value is a structurally-equal but distinct + // object — parsed from the wire or a deep link, or a nested-object default, + // which `collectSchemaDefaults` rebuilds per call. `SchemaNumberInput` has no + // such accidental protection, since a number compares by value. + // + // Either way the default belongs to what the field *opens* with, seeded + // above, not to what is re-imposed while it is being typed into. + useValueChange(value, (next) => { + if (!isSameJson(parseJsonDraft(draft), next)) { + setDraft(next === undefined ? "" : serializeJson(next)); + } + }); + + // Text that is present but does not parse yields no value, so the field would + // otherwise submit as absent while the user is still looking at what they + // typed. Saying so on the field keeps that from being silent; reporting it + // upward is what keeps it from being submittable. + const hasInvalidDraft = + draft.trim() !== "" && parseJsonDraft(draft) === undefined; + useDraftValidity(fieldName, !hasInvalidDraft, onValidityChange); + + return ( + { + setDraft(text); + onChange(parseJsonDraft(text)); + }} + /> + ); +} + +/** + * Whether a number field's draft is text that cannot be sent. + * + * Anything non-empty that `toNumericValue` declines is invalid: a half-typed + * `"-"`, and — the case that matters — an integer past `MAX_SAFE_INTEGER`, + * which is deliberately dropped rather than silently rounded (see + * `toNumericValue`). Both would otherwise submit as an absent argument while + * the text sits in the box. + */ +function hasInvalidNumericDraft(draft: string | number): boolean { + if (typeof draft === "number") { + return false; + } + return draft.trim() !== "" && toNumericValue(draft) === undefined; +} + +interface SchemaNumberInputProps { + fieldName: string; + label: string; + description?: string; + withAsterisk: boolean; + disabled: boolean; + /** + * Raw, not `resolveValue`-substituted — see `SchemaJsonFieldProps.value`. + * + * `null` is admitted alongside `undefined` because a nullable number schema + * produces it — by way of parent state, never by clearing the box — and the + * two are not interchangeable here: `undefined` means "no value supplied" + * and takes `defaultValue`, while `null` is a value and displays empty. + */ + value: number | null | undefined; + /** The schema `default`, used to seed the draft only (#2026). May be `null`. */ + defaultValue: number | null | undefined; min?: number; max?: number; allowDecimal: boolean; onChange: (value: number | undefined) => void; + onValidityChange: DraftValidityReporter; } /** @@ -126,22 +379,50 @@ interface SchemaNumberInputProps { * prop for the case it covers. */ function SchemaNumberInput({ + fieldName, value, + defaultValue, onChange, + onValidityChange, ...inputProps }: SchemaNumberInputProps) { - const [draft, setDraft] = useState(value ?? ""); + // `undefined` means "no value supplied", which is what the default is for. + // An explicit `null` is a value, so it must not be overwritten by a non-null + // default; it displays as the empty box `resolveValue` produced before. + // Hence the `undefined` test rather than `??`, matching `SchemaJsonField`. + // + // Clearing the box does *not* produce that `null` — `toNumericValue("")` + // reports `undefined`, which is the behavior the "passes undefined to + // onChange when a number field is cleared" test pins. A `null` reaches this + // field only from parent state: a value received from the server, restored + // from a deep link, or written by a caller for a nullable schema. + const [draft, setDraft] = useState( + (value === undefined ? defaultValue : value) ?? "", + ); + // Compared against the raw value, never the default-substituted one, for the + // reason spelled out on `SchemaJsonField` (#2026): clearing this box reports + // no value, which a defaulted field resolved straight back to its default — + // so the box refilled itself and the argument could not be emptied. useValueChange(value, (next) => { if (!Object.is(toNumericValue(draft), next)) { setDraft(next ?? ""); } }); + // Same split as the JSON field: text this client cannot send reports no value, + // so it has to say so on the field and report it upward, or the argument is + // dropped from a submission the user was allowed to make (#2020). + const invalidDraft = hasInvalidNumericDraft(draft); + useDraftValidity(fieldName, !invalidDraft, onValidityChange); + return ( { setDraft(next); onChange(toNumericValue(next)); @@ -174,6 +455,21 @@ export interface SchemaFormProps { * substitute — callers rebuild it every render, so its identity is unstable. */ resetKey?: string; + /** + * Called with `true` while any field is holding text that cannot be sent — + * unparseable JSON, or a number this client cannot represent exactly. Both + * report their value as `undefined`, which is indistinguishable from an empty + * field once it reaches `values`, so a caller cannot derive this from the + * values it already has (#2020). + * + * Gate the submit action on it: `hasMissingRequiredFields` covers a field with + * *no* value, and this covers a field whose visible text produced none. An + * empty optional field is unaffected — it stays valid and submittable. + * + * The callback is read through a ref, so an inline closure is fine — it is + * called when the answer changes, not on every render. + */ + onValidityChange?: (hasInvalidDraft: boolean) => void; } function getDefaultValue(fieldSchema: InspectorFormSchema): unknown { @@ -199,15 +495,70 @@ export function SchemaForm({ onChange, disabled = false, resetKey, + onValidityChange, }: SchemaFormProps) { const properties = schema.properties ?? {}; const requiredFields = schema.required ?? []; + // The names of fields currently holding unsendable text. Held here rather + // than in each field because only the form sees them all, and only the form + // knows when the last one cleared. + const [invalidFields, setInvalidFields] = useState>( + () => new Set(), + ); + + // Stable so a field's reporting effect subscribes once, not per render. The + // updater returns the previous set unchanged when nothing moved, which is + // what makes a nested form's inline callback safe to call repeatedly. + const reportFieldValidity = useCallback( + (fieldName, isValid) => { + setInvalidFields((previous) => { + if (previous.has(fieldName) === !isValid) { + return previous; + } + const next = new Set(previous); + if (isValid) { + next.delete(fieldName); + } else { + next.add(fieldName); + } + return next; + }); + }, + [], + ); + + // Read through a ref so the callback's identity is not a dependency. It has + // to be one or the other, and a *stable* dependency is what this needs: a + // nested form is handed a fresh closure every render, and re-running the + // effect for that alone would toggle the parent's state (cleanup, then + // re-report), re-render this form, and loop forever. + const notifyValidity = useRef(onValidityChange); + useEffect(() => { + notifyValidity.current = onValidityChange; + }); + + // The cleanup mirrors the one in `useDraftValidity`: a form that is no longer + // rendered holds no drafts, so an unmounted nested form must not leave the + // outer one blocked. It runs in the same commit as the re-report, so the + // transient `false` is never rendered. + const hasInvalidDraft = invalidFields.size > 0; + useEffect(() => { + notifyValidity.current?.(hasInvalidDraft); + return () => notifyValidity.current?.(false); + }, [hasInvalidDraft]); + function handleFieldChange(fieldName: string, fieldValue: unknown) { onChange({ ...values, [fieldName]: fieldValue }); } - function renderField(fieldName: string, fieldSchema: InspectorFormSchema) { + function renderField(fieldName: string, rawSchema: InspectorFormSchema) { + // Flatten a nullable union (`anyOf: [X, {type:"null"}]`, `type: [X,"null"]`) + // before dispatching. Every branch below tests a single `type` string, so + // without this an "optional and explicitly nullable" field — what Zod's + // `.nullish()` emits — matches nothing and falls through to the raw-JSON + // fallback, which is unusable for a value the user has to type (#1928). + const fieldSchema = normalizeNullableUnion(rawSchema); const isRequired = requiredFields.includes(fieldName); const label = fieldSchema.title ?? fieldName; const description = fieldSchema.description; @@ -223,6 +574,7 @@ export function SchemaForm({ withAsterisk={isRequired} disabled={disabled} data={toEnumData(fieldSchema.enum, fieldSchema.enumNames)} + clearable={isClearable(fieldSchema)} value={(rawValue as string) ?? null} onChange={(val) => handleFieldChange(fieldName, val)} /> @@ -230,11 +582,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 ( { oauthClientId: "", oauthClientSecret: "", oauthScopes: "", + oauthAuthorizationParams: [], enterpriseManaged: true, }); }); + // #2018 — the authorization-parameter rows ride the same onOAuthChange + // callback, so the modal folds them onto the settings object. + it("persists an added authorization-parameter row onto the settings", async () => { + const user = userEvent.setup(); + const onSettingsChange = vi.fn(); + renderWithMantine( + , + ); + await user.click(screen.getByRole("button", { name: "OAuth Settings" })); + await user.click(screen.getByRole("button", { name: "+ Add Parameter" })); + expect(onSettingsChange).toHaveBeenCalledWith( + expect.objectContaining({ + oauthAuthorizationParams: [{ key: "", value: "" }], + }), + ); + }); + it("calls onSettingsChange with a blank row when adding a root", async () => { const user = userEvent.setup(); const onSettingsChange = vi.fn(); diff --git a/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.tsx b/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.tsx index e391c240..869291b1 100644 --- a/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.tsx +++ b/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.tsx @@ -179,6 +179,10 @@ export function ServerSettingsModal({ oauthClientId: oauth.clientId, oauthClientSecret: oauth.clientSecret, oauthScopes: oauth.scopes, + // #2018: kept as rows (blank ones included) so a half-typed parameter + // survives a re-render; the drop-blank/omit-empty filtering happens on the + // way to disk. An emptied list persists as `[]`, which writes nothing. + oauthAuthorizationParams: oauth.authorizationParams, enterpriseManaged: oauth.enterpriseManaged ? true : undefined, // SEP-2350: persist only the non-default ('throw') so unset servers keep // the SDK's `reauthorize` behavior without writing a spurious field. diff --git a/clients/web/src/components/groups/ToolControls/ToolControls.tsx b/clients/web/src/components/groups/ToolControls/ToolControls.tsx index 4ad70d85..01ad1f04 100644 --- a/clients/web/src/components/groups/ToolControls/ToolControls.tsx +++ b/clients/web/src/components/groups/ToolControls/ToolControls.tsx @@ -15,6 +15,8 @@ import type { Tool } from "@modelcontextprotocol/client"; import type { ExcludedTool } from "@inspector/core/mcp/types.js"; import { ListChangedIndicator } from "../../elements/ListChangedIndicator/ListChangedIndicator"; import { ListLoadError } from "../../elements/ListLoadError/ListLoadError"; +import { MalformedItemsWarning } from "../../elements/MalformedItemsWarning/MalformedItemsWarning"; +import type { MalformedListItem } from "@inspector/core/mcp"; import { ListPaginationControls, type ListPaginationControlsProps, @@ -37,6 +39,12 @@ export interface ToolControlsProps { * A failed list load, surfaced above the list instead of leaving the panel * empty (which reads as "this server has none") (#1953). */ + /** + * Entries the client dropped from this list because they failed the MCP + * schema. Rendered as a warning above the list, which still shows the rest + * (#1909). + */ + malformedListItems?: MalformedListItem[]; loadError?: Error | null; /** Pagination controls (#1721). */ pagination: ListPaginationControlsProps; @@ -127,6 +135,7 @@ export function ToolControls({ searchText = "", listChanged, onRefreshList, + malformedListItems = [], loadError, pagination, onSearchChange, @@ -163,6 +172,11 @@ export function ToolControls({ /> + {filteredTools.map(({ tool, key }) => ( diff --git a/clients/web/src/components/groups/ToolDetailPanel/ToolDetailPanel.test.tsx b/clients/web/src/components/groups/ToolDetailPanel/ToolDetailPanel.test.tsx index feb434f2..cdad33ef 100644 --- a/clients/web/src/components/groups/ToolDetailPanel/ToolDetailPanel.test.tsx +++ b/clients/web/src/components/groups/ToolDetailPanel/ToolDetailPanel.test.tsx @@ -218,7 +218,13 @@ describe("ToolDetailPanel", () => { }); it("renders the Execute Tool button enabled when not executing", () => { - renderWithMantine(); + renderWithMantine( + , + ); const button = screen.getByRole("button", { name: "Execute Tool" }); expect(button).toBeInTheDocument(); expect(button).not.toBeDisabled(); @@ -231,6 +237,7 @@ describe("ToolDetailPanel", () => { , ); @@ -502,4 +509,61 @@ describe("ToolDetailPanel", () => { ).not.toBeInTheDocument(); }); }); + + // #2020: Execute used to be gated on `isExecuting` alone — this panel never + // called `hasMissingRequiredFields`, unlike the App and elicitation panels — + // so arguments that could not be sent were executed anyway and simply arrived + // absent at the server. + describe("submit gating (#2020)", () => { + const optionalJsonTool: Tool = { + name: "record_shipment", + inputSchema: { + type: "object", + // An array of no declared item type renders through the JSON editor, + // which is where a draft can fail to parse. + properties: { payload: { type: "array", title: "Payload" } }, + }, + }; + + it("disables Execute while a required argument has no value", () => { + renderWithMantine( + , + ); + expect( + screen.getByRole("button", { name: "Execute Tool" }), + ).toBeDisabled(); + }); + + it("enables Execute once the required argument is filled", () => { + renderWithMantine( + , + ); + expect( + screen.getByRole("button", { name: "Execute Tool" }), + ).not.toBeDisabled(); + }); + + it("disables Execute while an optional field holds text it cannot send", async () => { + const user = userEvent.setup(); + renderWithMantine( + , + ); + const execute = screen.getByRole("button", { name: "Execute Tool" }); + expect(execute).not.toBeDisabled(); + + // The draft never reaches `formValues` — the field reports `undefined` + // for text it cannot parse — so only the form's validity channel makes + // this visible to the gate. + const jsonInput = screen.getByLabelText(/Payload/); + await user.type(jsonInput, "x"); + expect(execute).toBeDisabled(); + + await user.clear(jsonInput); + expect(execute).not.toBeDisabled(); + }); + }); }); diff --git a/clients/web/src/components/groups/ToolDetailPanel/ToolDetailPanel.tsx b/clients/web/src/components/groups/ToolDetailPanel/ToolDetailPanel.tsx index fbb6d26d..46b92cfa 100644 --- a/clients/web/src/components/groups/ToolDetailPanel/ToolDetailPanel.tsx +++ b/clients/web/src/components/groups/ToolDetailPanel/ToolDetailPanel.tsx @@ -19,7 +19,10 @@ import type { ToolAnnotations, } from "@modelcontextprotocol/client"; import { resolveDisplayLabel } from "../../../utils/toolUtils"; -import { toFormSchema } from "../../../utils/jsonUtils"; +import { + hasMissingRequiredFields, + toFormSchema, +} from "../../../utils/jsonUtils"; import { getMirroredHeaderParams } from "@inspector/core/json/xMcpHeader.js"; import { AnnotationBadge } from "../../elements/AnnotationBadge/AnnotationBadge"; import { ProgressDisplay } from "../../elements/ProgressDisplay/ProgressDisplay"; @@ -213,6 +216,10 @@ export function ToolDetailPanel({ // a prior tool's hidden state doesn't carry over — mirrors how ToolsScreen // clears formValues on change. const [descriptionOpen, setDescriptionOpen] = useState(true); + // Text a field is holding that it could not turn into a value — unparseable + // JSON, or a number this client cannot send exactly. Reported by the form + // because the draft lives inside the field and never reaches `formValues`. + const [hasInvalidDraft, setHasInvalidDraft] = useState(false); const [prevToolName, setPrevToolName] = useState(name); if (name !== prevToolName) { setPrevToolName(name); @@ -245,6 +252,15 @@ export function ToolDetailPanel({ (taskSupport === "required" || ((taskSupport === "optional" || modernTasks) && runAsTask)); + // Two distinct reasons the arguments aren't sendable, and this panel used to + // check neither — Execute was gated on `isExecuting` alone, so a required + // argument left empty and a field full of unparseable text were both + // executable, and simply arrived at the server absent (#2020). + const executeDisabled = + isExecuting || + hasMissingRequiredFields(formSchema, formValues) || + hasInvalidDraft; + return ( @@ -321,6 +337,7 @@ export function ToolDetailPanel({ // so the form needs the tool name to drop another tool's // in-progress field text. See SchemaFormProps.resetKey. resetKey={name} + onValidityChange={setHasInvalidDraft} /> {progress && } @@ -341,7 +358,7 @@ export function ToolDetailPanel({