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..56959324 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -11,6 +11,7 @@ The Inspector ships as one package with three clients (**Web**, **CLI**, **TUI** - **Avoid double casts (`as unknown as T`).** They erase all type safety and usually mean the real type is being worked around. Prefer a type guard, a narrower single cast, or fixing the underlying type. If genuinely unavoidable (a documented gap in a third-party type, or bridging structurally-identical shapes TS can't relate), it **must** carry an inline comment justifying why it's safe and why nothing better exists. An unjustified `as unknown as` is not acceptable in review. - Prefer inference, type guards, and precise annotations over assertions. - An `_` prefix is the intentionally-unused marker (`argsIgnorePattern` / `varsIgnorePattern` / `caughtErrorsIgnorePattern`). +- **Never float a promise.** `@typescript-eslint/no-floating-promises` is `error` in all five ESLint scopes, so every promise must be awaited, `.catch(…)`-terminated, returned, or explicitly discarded with `void`. Prefer **holding and settling** it — a floated rejection surfaces in an unrelated file as an SDK-internal stack, which is how two un-held `callTool` calls failed the whole `npm run ci` chain (#1947). Reach for `void` only when the callee already owns its failures (it ends in a `catch`) or the caller genuinely cannot await (a sync effect body, an Ink key handler), and say which in a comment. In Storybook play functions, `expect(...)` from `storybook/test` is instrumented and returns a promise — it must be awaited. ## React and UI (web client) @@ -100,8 +101,9 @@ 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. +- **Build output is never a gate target.** Lint, format, and typecheck read first-party source only; everything a build writes (`clients/*/build`, `clients/web/dist`, `storybook-static`, `coverage`, `test-servers/build`, `core/**/{build,dist}`, `*.tsbuildinfo`) stays out via each scope's `globalIgnores`, `format` globs, and tsconfig `include`. Gating generated code reports defects in vendored third-party source that nobody can fix, and a rule promotion turns that warning into a `validate` failure (#2043). Flag a PR that adds a build location without ignoring it in the same change, that widens an ignore to silence a finding in first-party code, or that adds a build directory to a tsconfig `include` to make a generated `.d.ts` resolve. Note the coverage guards don't catch this — they assert source is *covered*, not that output is *excluded*. - **Every PR references an issue**, first body line `Closes #`. - **Every PR carries exactly one version label**, `v1` or `v2`, matching its base branch. - **Commits carry a `Signed-off-by:` trailer.** The DCO check is a hard merge gate and fails on a single unsigned commit; it matches the trailer against the author *or* committer, and skips only merge and bot commits. Use `git commit -s` — note `format.signOff` does *not* sign `git commit` (only `format-patch`). Repairing pushed commits means `git rebase HEAD~ --signoff` + `git push --force-with-lease`; remediation commits are not enabled on this repo. 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..0771175e 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,20 @@ 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; +│ │ # endpointOverrides.ts per-server +│ │ # authorization/token URL overrides — a fetch +│ │ # wrapper that rewrites the discovered AS +│ │ # metadata document, the one seam SDK v2 routes +│ │ # BOTH endpoints through (neither reaches the +│ │ # OAuthClientProvider) — #1906) │ │ ├── browser/ # Browser-side OAuth (sessionStorage, BrowserNavigation) │ │ ├── node/ # Node-side OAuth (NodeOAuthStorage, OAuthCallbackServer, │ │ │ # runner-interactive-oauth loopback callback flow) @@ -52,12 +68,51 @@ 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 +│ │ # (uriTemplate.ts: RFC 6570 parse/classify/expand. +│ │ # The ONE expander for the web Resources form and +│ │ # readResourceFromTemplate (TUI); the CLI is NOT a +│ │ # consumer — it has no template form and passes an +│ │ # already-expanded --uri to readResource. Both +│ │ # consumers derive their FORM FIELDS from it too +│ │ # (clients/tui uriTemplateToForm), +│ │ # which is what makes the sharing real: a form +│ │ # submits under the names it rendered, so a mangled +│ │ # name silently drops the value at expansion. The +│ │ # SDK's UriTemplate is used only to VALIDATE; its +│ │ # expander is wrong for `{a,b}` (raw-joined), +│ │ # `{;id}` (operator missing), `{id:3}` (modifier in +│ │ # the name), `{+v}`/`{#v}` (encodeURI mangles `[`/`]` +│ │ # and double-encodes pct-triplets), and `{v}` +│ │ # (encodeURIComponent leaves `!'()*` bare). +│ │ # Requiredness is per EXPRESSION, not per variable — +│ │ # requiredGroups + hasRequiredValues. A template that +│ │ # cannot expand ({id:abc}, {}, {a,}) WITHHOLDS the read: +│ │ # tryExpandUriTemplate returns the reason as a value, and +│ │ # the panel disables Read Resource rather than sending the +│ │ # raw template — expandUriTemplate's raw-template fallback +│ │ # is for DISPLAY (the preview) only. An UNDEFINED +│ │ # variable omits its expression; one defined as "" +│ │ # expands (`x{?q}` → `x?q=`), so each FORM — not the +│ │ # expander — drops its untouched blanks via +│ │ # definedValues — #1919; +│ │ # 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 +151,19 @@ 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; resolve-node-bin.mjs resolves a +│ # package's bin through its own package.json, so the +│ # verify/smoke scripts spawn it with `process.execPath` +│ # rather than the `npx` `.cmd` shim Node refuses to +│ # spawn shell-free on Windows. pack-and-verify.mjs is +│ # the exception — its children are `npm` itself and the +│ # installed `.bin` shim, neither resolvable that way, so +│ # it keeps a Windows shell and quotes its generated +│ # paths via win-shell-args.mjs — #1939). 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 +528,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 +730,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. @@ -678,6 +748,19 @@ The ⚠️ option-deletion hazard, the snapshot rule, and the recovery recipe ab - `smoke:cli` (`scripts/smoke-cli.mjs`) drives `mcp-inspector --cli` through the built launcher against the bundled stdio test server via a temp `--catalog`: it asserts `tools/list` returns the server's tools (real connect over stdio), the default writable catalog is seeded empty on first run, a missing read-only `--config` errors without seeding, and `--catalog` + `--config` is rejected. `smoke:tui` (`scripts/smoke-tui.mjs`) launches `mcp-inspector --tui --catalog ` and asserts the Ink app renders its first frame (the "MCP Servers" panel) within a timeout, then SIGTERMs it — a shallow boot/render check, not full interaction. **`smoke:tui` is local-only: it self-skips when `process.env.CI` is set**, because the Ink TUI needs a real TTY (raw mode) that headless CI lacks — so run it (via `npm run smoke`) on your own machine before pushing. Both build `test-servers/build` on demand if it's missing. - Storybook play-function tests (`clients/web` `test:storybook`) run in headless Chromium via `@vitest/browser-playwright` (~10s). They are part of `npm run ci` (which installs Playwright chromium first); kept out of `validate` because they need the browser binary and are slower than the unit suite. +### Build output is never a gate target + +**No gate — lint, format, or typecheck — may read generated output.** The gated surface is first-party source only: `clients/*/src`, `clients/*/__tests__`, `clients/web/{server,.storybook}`, each client's top-level configs, `core/`, `test-servers/src`, `scripts/`, and the root shared files. Everything a build writes is out of scope: `clients/*/build` (the tsup/tsc bundles), `clients/web/dist` (the Vite SPA), `clients/web/storybook-static`, `clients/*/coverage`, `test-servers/build`, `core/**/{build,dist}`, and any `*.tsbuildinfo`. Each scope states this itself — `globalIgnores([...])` in every `eslint.config.js`, the `format`/`format:check` globs in each `package.json`, and a tsconfig `include` that names source directories rather than the package root. + +Why it matters, given that these paths are all gitignored and the findings are usually warnings: + +- **It reports defects nobody can fix.** A bundle vendors third-party code, so a rule that fires inside it names a problem in someone else's source. `clients/web` shipped this for a while: `build` was missing from its `globalIgnores` while its three sibling clients had it, so the client's *only* lint output was an unused-`eslint-disable` warning from inside the vendored `undici` (#2043). +- **A warning becomes a gate failure without warning.** `reportUnusedDisableDirectives` is a warning by default and a rule promotion — or any new rule a bundled dependency happens to trip — turns it into a `validate` failure on a file nobody wrote. #1959 (enabling `no-floating-promises` across all five scopes) is exactly that kind of change. +- **It trains people to skim the channel.** A scope whose lint is never clean has no signal left in it, and the real warning added later lands where everyone has learned to look past. +- **It is wasted work on every run.** `lint` runs inside `validate`, the fast inner-loop check, and the web runner bundle alone is ~1.2MB of generated JS. + +The two coverage guards do **not** catch this, and adding a third is not the fix. `verify:format-coverage` and `verify:typecheck-coverage` assert that first-party source is *covered*; neither asserts that generated output is *excluded* — an asymmetry that is deliberate, since a guard can't distinguish "generated" from "source" without being told, and the ignore lists are already that statement. So this class drifts silently and the check is a human one: **when a build starts writing to a new location, add it to that scope's ignore list in the same change.** The reverse of the guards' rule also holds — never widen an ignore to silence a finding in first-party code, and never add a build directory to a tsconfig `include` to make a generated `.d.ts` resolve (import the source, or fix the build's types). + ### Typescript instructions - Use TypeScript for all new code @@ -690,6 +773,12 @@ The ⚠️ option-deletion hazard, the snapshot rule, and the recovery recipe ab - Use type guards and type assertions to handle potential type mismatches and ensure type safety - Take advantage of TypeScript's advanced features like generics, type aliases, and conditional types to write more expressive and reusable code - Regularly review and refactor TypeScript code to ensure it remains well-structured and adheres to evolving best practices +- **NEVER leave a promise floating.** `@typescript-eslint/no-floating-promises` is enabled at `error` in **all five** ESLint scopes — `clients/{web,cli,tui,launcher}` and the root `core/` + shared gate (#1959). Every promise must be awaited, returned, terminated with `.catch(…)`, or explicitly discarded with the `void` operator. + - **The class it catches is invisible at review time.** A floated call reads like an awaited one minus four characters, and the unhandled rejection it produces surfaces somewhere else entirely — a different test, a different file, a stack pointing at SDK internals. Two un-held `client.callTool(...)` promises made `npm run ci` unpassable in #1947: `disconnect()` rejected them with `Connection closed`, the unhandled rejection failed the whole vitest run, and the chain aborted at `coverage`, silently skipping `verify:build-gate`, `smoke`, and Storybook. Attributing it took a full investigation; the fix was two lines. + - **Prefer holding and settling the promise.** `void` is an escape hatch, not a fix — it is visible at review time (strictly better than nothing) but still discards the rejection. Reach for it only when the callee already owns its failures (it ends in a `catch` that surfaces the message) or the caller genuinely cannot await — a synchronous `useEffect` body, an Ink `useInput` key handler, a Hono `stream.onAbort` listener. Say **which** of those it is in a one-line comment; an unexplained `void` is a review finding. Where the callee does *not* own its failures, give it a `catch` rather than voiding the call (see `handleDisconnect` in `clients/tui/src/App.tsx`), or terminate with `.catch(…)` at the call site (see `open(url)` in `clients/web/server/{server,vite-hono-plugin}.ts`). + - **In Storybook play functions, `expect(...)` from `storybook/test` returns a promise.** Storybook instruments it so the interactions panel can trace each assertion, so every `expect` in a play function is awaited — as is any shared helper that wraps one (`src/test/scrollAreaStoryAssertions.ts` is `async` for this reason). + - **The rule is type-aware, so each scope's ESLint config carries a parser project.** Each client's config must name **every leaf project covering its lint surface**, since the parser needs a program that literally *contains* the file — for cli, tui, and launcher that is the two they already typecheck (`tsconfig.json` + `tsconfig.test.json`, `src` in the first and `__tests__` only in the second), and for **web it is four** (`tsconfig.app.json`, `tsconfig.node.json`, `tsconfig.storybook.json`, `tsconfig.test.json`) — web's `tsconfig.json` is a solution file with `files: []`, so naming it alone would contain nothing. Adding a leaf project to a client means adding it here too. The root config instead points at **`tsconfig.lint.json`**, which exists solely to give the parser a program covering `core/**`, `test-servers/src/**`, and `vitest.shared.mts` — none of which is rooted in a tsconfig of its own. That file emits nothing and gates nothing: type *checking* for those sources stays where it was (`core/` through `clients/web`'s `tsc -b`, `test-servers/src` through `clients/cli`'s test project). It sets `moduleResolution: bundler` deliberately — `core/` uses extensionless relative imports, which NodeNext fails to resolve, degrading every import to `any` so the rule silently stops seeing promises at all. **Its `include` must stay a superset of the root config's type-aware `files` globs** — a file the lint block matches but the project omits fails outright with "was not found in any of the provided project(s)" rather than being checked, so widening one means widening the other (that is why the `include` carries `test-servers/src/**/*.tsx`, which nothing has produced yet). Note also that a tsconfig `include` does **not** expand braces — `core/**/*.{ts,tsx}` matches nothing; list the extensions separately. + - Type-aware linting costs real time: `clients/web`'s `eslint .` went from ~8s to ~19s, and `lint` runs inside `validate`, the fast inner-loop check. That is the price of the guarantee; if it needs reducing later, narrowing the projects each scope loads is the lever, not dropping the rule. ## Web source layout: `src/lib` vs `src/utils` 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..2c420c00 100644 --- a/README.md +++ b/README.md @@ -30,11 +30,13 @@ inspector/ │ ├── tui/ # TUI client (Ink + React, tsup bundle) │ └── launcher/ # Shared launcher — provides the `mcp-inspector` bin, dispatches to web/cli/tui ├── core/ # Shared code consumed via the `@inspector/core` alias (no package.json) -│ ├── auth/ # OAuth: providers, discovery, storage, mid-session recovery (browser/node/remote backends) +│ ├── auth/ # OAuth: providers, discovery, storage, endpoint overrides, 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 +│ ├── mcp/ # InspectorClient runtime, state stores, transports, config import, +│ │ # and the RFC 6570 URI-template helpers the web form and TUI expand through │ ├── node/ # Node-only shared helpers: version reader, hostUrl (host normalize/canonicalize + all-interfaces/loopback detection) │ ├── react/ # React hooks over the state stores │ └── storage/ # File I/O helpers for the OAuth persist backends @@ -69,7 +71,7 @@ npm install # root install; postinstall cascades into every client The cascade (`scripts/install-clients.mjs`) is dev-only — it exits early when the package is installed as a dependency, and the published tarball ships only each client's `build/`, so end users are unaffected. Set `INSPECTOR_SKIP_CLIENT_INSTALL=1` to skip it. -**Where a dependency is declared.** The MCP SDK packages (`@modelcontextprotocol/client`, `core`, `server`, `server-legacy`, `ext-apps`) live in the **root** `package.json` only — never in a client's. Node resolution walks up, so the root install is on every client's chain, and the root manifest is already what the published tarball resolves against. Declaring them per client installs a second copy that can drift from the root's, which is how two versions of `ext-apps` (and of the transitive v1 `@modelcontextprotocol/sdk`) ended up in the tree before [#1970](https://github.com/modelcontextprotocol/inspector/issues/1970) — and a second copy of `client`/`core` is the failure `vitest.shared.mts` carries a `dedupe` workaround for. The same root-only placement holds for anything reached solely through root-owned code with no manifest of its own (`test-servers/src`, `core/`), and `vitest.shared.mts` aliases those to the repo root — `express` and `yaml`, both reached through `test-servers/src`, are the two today. **Whether such a package is a `dependency` or a `devDependency` follows from who consumes it at runtime, not from where it is declared:** anything `core/` imports at runtime must be a root **`dependency`**, because the client builds externalize npm packages and a published install resolves them from the root manifest, where devDependencies are absent. `express` is test-only and is a devDependency; `yaml` currently sits in `dependencies`. +**Where a dependency is declared.** The MCP SDK packages (`@modelcontextprotocol/client`, `core`, `server`, `server-legacy`, `ext-apps`) live in the **root** `package.json` only — never in a client's. Node resolution walks up, so the root install is on every client's chain, and the root manifest is already what the published tarball resolves against. Declaring them per client installs a second copy that can drift from the root's, which is how two versions of `ext-apps` (and of the transitive v1 `@modelcontextprotocol/sdk`) ended up in the tree before [#1970](https://github.com/modelcontextprotocol/inspector/issues/1970) — and a second copy of `client`/`core` is the failure `vitest.shared.mts` carries a `dedupe` workaround for. The same root-only placement holds for anything reached solely through root-owned code with no manifest of its own (`test-servers/src`, `core/`), and `vitest.shared.mts` aliases those to the repo root — `express` and `yaml`, both reached through `test-servers/src`, are the two today. **Whether such a package is a `dependency` or a `devDependency` follows from who consumes it at runtime, not from where it is declared:** anything `core/` imports at runtime must be a root **`dependency`**, because the client builds externalize npm packages and a published install resolves them from the root manifest, where devDependencies are absent. `express` is test-only and is a devDependency; `yaml` currently sits in `dependencies`. **`vite` and `@vitejs/plugin-react` are root `dependencies` for the same reason, not by mistake** — they look like build tooling, but `clients/web/server/start-vite-dev-server.ts` imports them at runtime for `mcp-inspector --web --dev`, and `clients/web/tsup.runner.config.ts` lists both as `external`, so a published install resolves them from the root manifest. Moving them to `devDependencies` would break `--web --dev` for consumers (and the on-demand `vite build` in `ensure-web-build.ts`) while passing every local check. It does mean they show up under `npm audit --omit=dev`, which is a feature: they really are in the production tree. ## Running during development @@ -139,12 +141,14 @@ Each config below is a ready-made server for exercising one feature by hand. Loa | ----------------------------------------- | -------------------------------------------------- | ---------------------------------------------------------------------- | | `mcp-app-http.json` **(legacy era)** | An MCP App (UI resource + app tool) in the Apps tab | [#1859](https://github.com/modelcontextprotocol/inspector/issues/1859) | | `modern-mrtr-http.json` | A single MRTR round-trip | — | -| `mrtr-showcase-http.json` | Every MRTR preset in one server | — | +| `mrtr-showcase-http.json` | Every MRTR preset in one server | [#1860](https://github.com/modelcontextprotocol/inspector/issues/1860) | | `modern-network-http.json` | Network tab: `Mcp-*` headers + error taxonomy | [#1628](https://github.com/modelcontextprotocol/inspector/issues/1628) | | `xmcpheader-modern-http.json` | Tools tab: `x-mcp-header` mirroring and exclusions | [#1632](https://github.com/modelcontextprotocol/inspector/issues/1632) | | `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) | +| `rfc6570-templates-http.json` | Resources tab: RFC 6570 resource-template expansion | [#1919](https://github.com/modelcontextprotocol/inspector/issues/1919) | | `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) | @@ -173,8 +177,21 @@ The Inspector drives MRTR manually (`inputRequired: { autoFulfill: false }`), so | `mrtr_sample` | Embedded sampling → the Sampling panel | | `mrtr_roots` | Embedded `roots/list`, auto-answered silently from configured roots (no modal) | | `mrtr_edge` | An `inputRequests`-only round, then a `requestState`-only round | +| `mrtr_empty` | Completes with an empty result — no `content`, no `structuredContent` | | `mrtr_loop` | Never completes → trips the `MRTR_MAX_ROUNDS` bound | +Run `mrtr_empty` and answer its single elicitation: the Protocol tab groups the +exchange as an MRTR conversation ending **COMPLETE**, and the Results panel says +**"Empty result — The tool call completed successfully and returned no content."** +On the broken build that same result rendered as **"No results yet"**, the panel's +pre-run placeholder ([#1860](https://github.com/modelcontextprotocol/inspector/issues/1860)) — +so a call the user had just watched succeed read as a call that never ran. An +empty `content` array with no `structuredContent` is a legal `CallToolResult`, +and the panel only ever mounts once a result exists, so the placeholder wording +could not be true there. (The neighbouring half of the same gap — a result whose +payload lives only in `structuredContent` — was closed by +[#1908](https://github.com/modelcontextprotocol/inspector/issues/1908).) + > The legacy `collect_elicitation` preset calls `server.elicitInput`, which errors on the 2026-07-28 leg — server→client requests aren't allowed there. MRTR is the modern replacement. #### Network tab — standardized headers and error taxonomy @@ -227,6 +244,46 @@ 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. + +#### RFC 6570 resource templates + +`rfc6570-templates-http.json` serves two resource templates straight out of [#1919](https://github.com/modelcontextprotocol/inspector/issues/1919) — `events_by_topic` (`foobar://events/{topic}`) and `events_by_query` (`foobar://events{?topic}`) — each echoing the URI it was matched against, plus a plain `foobar://events` resource (see below). Plain streamable-HTTP; connect with the **default (legacy)** protocol era. + +Open the Resources tab and pick **events_by_topic**, then enter `foo/bar`. The request must go out as `foobar://events/foo%2Fbar`, and the result echoes back the URI the server matched. On the broken build the value was spliced in raw, so the slash created a second path segment and the SDK's matcher answered `-32602 Resource not found: foobar://events/foo/bar` — the exact failure in the issue. The same holds for `?`, `#`, `%`, spaces, and non-ASCII text. + +**events_by_query** is the half that was invisible: the old `/\{(\w+)\}/g` scan could not see an expression carrying an operator, so no `topic` input was rendered at all. It now appears, marked **Optional** — RFC 6570 drops the whole expression when the variable is undefined, so reading with the field blank requests `foobar://events`, and filling it in requests `foobar://events?topic=foo%2Fbar`. The URI preview beside the title shows the partially-expanded form as you type, leaving unfilled expressions standing as written. + +> The plain `foobar://events` resource is registered deliberately, not as filler. The SDK's `UriTemplate.match()` compiles `{?topic}` to a **required** `\?topic=([^&]+)`, so a template alone cannot serve the blank read — `match("foobar://events")` returns `null`. A real server exposes the unfiltered collection as its own resource; the showcase does the same so that step actually resolves. + +The web client and the TUI expand through one shared helper, [`core/mcp/uriTemplate.ts`](./core/mcp/uriTemplate.ts) — the web Resources form directly, the TUI via `InspectorClient.readResourceFromTemplate` — and both derive their **form fields** from its parser too, which is the half that makes the sharing real: a form submits values under the names it rendered, so a parser that mangles a name silently drops the value at expansion time. (The CLI is not a consumer: it has no template form, and its `resources/read` passes the already-expanded `--uri` straight through.) + +The SDK's `UriTemplate` is still used, but only to *validate* a template (constructing it is what rejects an unclosed expression). Its expander is not, because it is incomplete in five ways — each measured against the pinned SDK, not inferred: + +| Shape | SDK behavior | +| --- | --- | +| `{a,b}` | raw-joins the values — no encoding, operator prefix dropped | +| `{;id}` | `;` is missing from its operator list, so the variable parses as `;id` | +| `{id:3}` | the prefix modifier is folded into the name, giving `id:3` | +| `{+v}` / `{#v}` | `encodeURI` mangles reserved `[`/`]` (`[::1]` → `%5B::1%5D`) and double-encodes pct-triplets (`%2F` → `%252F`) | +| `{v}` | `encodeURIComponent` leaves the sub-delims `!'()*` bare, which RFC 6570 requires encoded | + +The `;` and `:3` rows are the ones a user sees directly: on the SDK's parse the form renders fields literally labelled `;id` and `id:3`. The `+`/`#` row is silent corruption rather than over-escaping — an IPv6 literal or an already-encoded path arrives at the server altered. + +A template that cannot be expanded at all — an out-of-grammar modifier (`{id:abc}`), or an expression declaring no variable (`{}`, `{a,}`, `{?}`) — **withholds the read** rather than sending something. Pick **events_malformed** (`foobar://events/{topic:abc}`) to see it: Read Resource is disabled, the reason is printed under the form, and the preview shows the template as the server declared it. The alternative is worse than it looks: `x://{}` would otherwise expand to `x://` with no inputs rendered, so the form's "everything required is filled" check passes vacuously and it reads a URI that is not the template the server published. + +Literals are pct-encoded on expansion too (RFC 6570 §3.1): `café/{var}` sends `caf%C3%A9/value`, not raw UTF-8 in the path — something the SDK's expander does not do either. And the *names* a template may use are RFC 6570's `varchar` plus a labelled tolerance for `-` and `~`: the conformance suite rejects `{default-graph-uri}`, but real servers publish such names and the SDK's matcher round-trips them, so the Inspector expands them and marks the variable `conforming: false` rather than refusing a resource that demonstrably works. + +An **undefined** variable is what omits its expression — a variable defined as the empty string expands (`x{?q}` gives `x?q=`, `x{;q}` gives `x;q`, per RFC 6570 §3.2.7). The expander honors that distinction, so a caller such as `readResourceFromTemplate` can request either URI. Collapsing the two is a *form* concern, not a template one: both clients seed every declared variable with `""` and a text input cannot express "defined but empty", so each form drops its blanks (`definedValues`) on the way in. + +Requiredness is a property of the **expression**, not the variable: RFC 6570 drops undefined names from a multi-name expression, so `{a,b}` with only `a` filled is expandable and a form must not block it. `requiredGroups` returns one entry per non-omittable expression and `hasRequiredValues` asks that each be satisfied by any one of its names — which no per-variable flag can express once a name recurs across expressions (`{a,b}{a,c}` is satisfied by filling `b` and `c`). + #### 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,19 +341,21 @@ 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, plus `scripts/lib/resolve-node-bin.test.mjs` — the cross-platform bin resolver (#1939), pinned against the real `bin`/`exports` shapes of the packages the scripts actually spawn. 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). | Per-client scripts exist too (`validate:web`, `coverage:cli`, `smoke:tui`, …), plus root `validate:core` / `format:core` for the shared `core/` package, `format:scripts` for the root `scripts/` tooling, and `format:shared` / `lint:shared` for the root "shared" surface (`test-servers/src/**`, `vitest.shared.mts`, the root `eslint.config.js`). Run `npm run format` before committing — the root `format` fixes `core/`, the root `scripts/`, the shared surface, and every client; `validate` runs the non-fixing `format:check` and fails CI on any unformatted file. +**Linting is type-aware.** All five ESLint scopes (`clients/{web,cli,tui,launcher}` plus the root `core/` + shared gate) enable `@typescript-eslint/no-floating-promises` at `error`, so a promise that is neither awaited, returned, `.catch(…)`-terminated, nor explicitly discarded with `void` fails `lint` — and therefore `validate` ([#1959](https://github.com/modelcontextprotocol/inspector/issues/1959)). The rule needs type information, so each scope's config names a parser project; the root scope's is **`tsconfig.lint.json`**, a lint-only project covering `core/**`, `test-servers/src/**`, and `vitest.shared.mts`, which have no tsconfig of their own. It emits nothing and changes no typecheck — but a new first-party TS location added to the root lint scope must be added to its `include`. See **TypeScript instructions** in [`AGENTS.md`](./AGENTS.md) for when `void` is acceptable. + For the full testing rules — the ≥90% per-file gate, where test files live, the unit vs. integration vs. storybook projects, and the `v8 ignore` policy — see [`AGENTS.md`](./AGENTS.md). ## Publishing @@ -326,16 +385,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 +434,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/cli/__tests__/run-method.test.ts b/clients/cli/__tests__/run-method.test.ts index 783ba60b..d8599223 100644 --- a/clients/cli/__tests__/run-method.test.ts +++ b/clients/cli/__tests__/run-method.test.ts @@ -258,18 +258,29 @@ describe("runMethod", () => { ); expect(stdout.trim().split("\n")).toHaveLength(2); + // Invoke only the SIGINT handler consumeMethodOutcome registers, rather + // than broadcasting `process.emit("SIGINT")` process-wide: `atomically` + // (loaded via core's store-io) pulls in `when-exit`, whose module-level + // `process.once("SIGINT")` handler re-raises the signal with + // `process.kill(process.pid, "SIGINT")` — on Windows an unconditional + // TerminateProcess that kills the vitest worker fork mid-run (#1941). + const listenersBefore = new Set(process.listeners("SIGINT")); const streamPromise = consumeMethodOutcome( { kind: "stream", label: "t", start: (write) => { write({ hi: true }); - queueMicrotask(() => process.emit("SIGINT")); return () => {}; }, }, {}, ); + const added = process + .listeners("SIGINT") + .filter((listener) => !listenersBefore.has(listener)); + expect(added).toHaveLength(1); + for (const listener of added) listener("SIGINT"); await streamPromise; expect(stdout).toContain('"hi":true'); } finally { diff --git a/clients/cli/eslint.config.js b/clients/cli/eslint.config.js index 9407f997..699b202f 100644 --- a/clients/cli/eslint.config.js +++ b/clients/cli/eslint.config.js @@ -16,4 +16,22 @@ export default defineConfig([ globals: globals.node, }, }, + { + // Type-aware pass for `no-floating-promises` (#1959). The rule needs type + // information, which `tseslint.configs.recommended` does not provide, and + // the parser needs a project that literally contains the linted file — so + // both of this client's tsconfig projects are listed, exactly as + // `npm run typecheck` runs them (`src` is in the first, `__tests__` only + // in the second). + files: ["**/*.ts"], + languageOptions: { + parserOptions: { + project: ["./tsconfig.json", "./tsconfig.test.json"], + tsconfigRootDir: import.meta.dirname, + }, + }, + rules: { + "@typescript-eslint/no-floating-promises": "error", + }, + }, ]); diff --git a/clients/cli/package-lock.json b/clients/cli/package-lock.json index f81d37e6..30a14836 100644 --- a/clients/cli/package-lock.json +++ b/clients/cli/package-lock.json @@ -3558,9 +3558,9 @@ } }, "node_modules/nanoid": { - "version": "3.3.16", - "resolved": "https://registry.npmjs.org/nanoid/-/nanoid-3.3.16.tgz", - "integrity": "sha512-bzlKTyNJ7+LdGIIwy8ijFpIqEQIvafahV7eYykJ8Cvh42EdJeODoJ6gUJXpQJvej1BddH8OqTXZNE/KfbWAu8Q==", + "version": "3.3.18", + "resolved": "https://registry.npmjs.org/nanoid/-/nanoid-3.3.18.tgz", + "integrity": "sha512-DTg4MJbGMWkfi6VZFdNt2/caMbQy4Ou+Op/hJQvGEWcnVfoA1QA+xzRKAzw9jD6+GVOOeYr/mIcuDSdug6F6+w==", "dev": true, "funding": [ { diff --git a/clients/launcher/eslint.config.js b/clients/launcher/eslint.config.js index 535689a1..b13efaba 100644 --- a/clients/launcher/eslint.config.js +++ b/clients/launcher/eslint.config.js @@ -16,4 +16,19 @@ export default defineConfig([ globals: globals.node, }, }, + { + // Type-aware pass for `no-floating-promises` (#1959) — see the CLI config + // for the reasoning. Both tsconfig projects are listed because `src` and + // `__tests__` live in different ones. + files: ["**/*.{ts,mts}"], + languageOptions: { + parserOptions: { + project: ["./tsconfig.json", "./tsconfig.test.json"], + tsconfigRootDir: import.meta.dirname, + }, + }, + rules: { + "@typescript-eslint/no-floating-promises": "error", + }, + }, ]); diff --git a/clients/launcher/package-lock.json b/clients/launcher/package-lock.json index 93c1e23b..e9d690fc 100644 --- a/clients/launcher/package-lock.json +++ b/clients/launcher/package-lock.json @@ -1959,9 +1959,9 @@ "license": "MIT" }, "node_modules/nanoid": { - "version": "3.3.16", - "resolved": "https://registry.npmjs.org/nanoid/-/nanoid-3.3.16.tgz", - "integrity": "sha512-bzlKTyNJ7+LdGIIwy8ijFpIqEQIvafahV7eYykJ8Cvh42EdJeODoJ6gUJXpQJvej1BddH8OqTXZNE/KfbWAu8Q==", + "version": "3.3.18", + "resolved": "https://registry.npmjs.org/nanoid/-/nanoid-3.3.18.tgz", + "integrity": "sha512-DTg4MJbGMWkfi6VZFdNt2/caMbQy4Ou+Op/hJQvGEWcnVfoA1QA+xzRKAzw9jD6+GVOOeYr/mIcuDSdug6F6+w==", "dev": true, "funding": [ { diff --git a/clients/tui/__tests__/App.test.tsx b/clients/tui/__tests__/App.test.tsx index d70b9c27..a66fe603 100644 --- a/clients/tui/__tests__/App.test.tsx +++ b/clients/tui/__tests__/App.test.tsx @@ -507,6 +507,9 @@ function renderApp(servers: Record) { async function mount(servers: Record) { const r = renderApp(servers); await tick(); + // The mount commit's effect flush can still be queued behind this tick, and + // this write is the one that absorbs the dropped first keypress. + await settleInputHandlers(); r.stdin.write("x"); await tick(); return r; @@ -532,6 +535,11 @@ const ENTER = "\r"; */ async function press(r: RenderResult, keys: string[]) { for (const k of keys) { + // Same hazard `waitUntil` guards against, at the other end: a caller can + // reach here on a turn that still has React's passive-effect flush queued + // (e.g. straight after a plain `tick`, or after an earlier key committed a + // render), so settle before every write rather than only after a poll. + await settleInputHandlers(); r.stdin.write(k); await tick(); await tick(); @@ -552,9 +560,29 @@ async function press(r: RenderResult, keys: string[]) { */ const POLL_TRIES = 100; +/** + * One check-phase turn, queued BEHIND React's already-scheduled passive-effect + * flush. A frame observed by a poll predicate is written during React's + * COMMIT, but ink re-arms its useInput listeners in the passive-effect flush + * React schedules (via setImmediate in Node) during that same commit. Node's + * event loop runs the timers phase before the check phase, so a 25ms poll + * tick can observe the new frame and let the test write the next keypress + * BEFORE that flush has run — the key is then dispatched to the previous + * commit's stale useInput closures (where e.g. pendingStepUp is still null) + * and silently swallowed (#1942). Yielding one setImmediate turn after the + * predicate passes sequences the next stdin write after the flush (FIFO + * within the check queue), so "frame visible" once again implies "input + * handlers armed". + */ +const settleInputHandlers = () => + new Promise((resolve) => setImmediate(resolve)); + async function waitUntil(predicate: () => boolean, tries = POLL_TRIES) { for (let i = 0; i < tries; i++) { - if (predicate()) return; + if (predicate()) { + await settleInputHandlers(); + return; + } await tick(); } } @@ -626,6 +654,23 @@ afterEach(() => { while (mounted.length) mounted.pop()?.unmount(); }); +// Pins the synchronization contract the OAuth step-up assertions depend on +// (#1942). The `setImmediate` sentinel below stands in for React's pending +// passive-effect flush — the turn where ink re-arms `useInput`. If `waitUntil` +// ever returns without yielding a check-phase turn, the sentinel has not run +// and this fails, instead of the regression resurfacing as a differently-named +// flaky OAuth test under coverage instrumentation. +describe("test helpers", () => { + it("waitUntil settles input handlers before resolving", async () => { + let flushed = false; + setImmediate(() => { + flushed = true; + }); + await waitUntil(() => true); + expect(flushed).toBe(true); + }); +}); + describe("App (foundation)", () => { it("renders the server list with the MCP Servers header", async () => { const r = renderApp(stdioServer()); @@ -661,6 +706,91 @@ describe("App (foundation)", () => { expect(h.disconnect).toHaveBeenCalled(); }); + it("surfaces a disconnect failure instead of floating the rejection", async () => { + // 'd' is a key handler, so it cannot await handleDisconnect — the handler + // has to own the failure itself or it escapes as an unhandled rejection + // and fails the whole run from somewhere else (#1959). + // The banner is deliberately independent of connection status: a rejected + // disconnect leaves the status "connected", so anything gated on + // `status === "error"` would never be seen. + h.ctrl.status = "connected"; + h.disconnect.mockRejectedValue(new Error("discfail")); + const r = await mount(oneStdio()); + r.stdin.write("d"); + await expectFrame(r, "Disconnect failed: discfail"); + }); + + it("owns a non-Error disconnect rejection too", async () => { + // The catch stringifies a non-Error rejection rather than reading + // `.message` off it; a throw here would escape the same way. + h.ctrl.status = "connected"; + h.disconnect.mockRejectedValue("plainstring"); + const r = await mount(oneStdio()); + r.stdin.write("d"); + await expectFrame(r, "Disconnect failed: plainstring"); + }); + + it("clears a disconnect failure once a retry succeeds", async () => { + // A stale banner would keep reporting a failure the user has since fixed. + h.ctrl.status = "connected"; + h.disconnect.mockRejectedValueOnce(new Error("discfail")); + const r = await mount(oneStdio()); + r.stdin.write("d"); + await expectFrame(r, "Disconnect failed: discfail"); + r.stdin.write("d"); + await waitUntil(() => !(r.lastFrame() ?? "").includes("Disconnect failed")); + expect(r.lastFrame() ?? "").not.toContain("Disconnect failed"); + }); + + it("drops a disconnect rejection that lands after the user switched servers", async () => { + // The stale attempt is the one that usually rejects, so without an + // attempt token server alpha's failure would surface in beta's header. + h.ctrl.status = "connected"; + let rejectDisconnect: (err: Error) => void = () => {}; + h.disconnect.mockImplementationOnce( + () => + new Promise((_resolve, reject) => { + rejectDisconnect = reject; + }), + ); + const r = await mount(stdioServer()); + r.stdin.write("d"); // alpha's disconnect starts, and hangs + await tick(); + await press(r, [DOWN]); // switch to beta + await expectFrame(r, "beta"); + rejectDisconnect(new Error("stale-alpha-failure")); + // Give the rejection every chance to be published before asserting it + // wasn't — a bare tick would pass even with the guard removed. + await waitUntil(() => + (r.lastFrame() ?? "").includes("stale-alpha-failure"), + ); + expect(r.lastFrame() ?? "").not.toContain("stale-alpha-failure"); + }); + + it("drops a stale disconnect rejection across an A → B → A round trip", async () => { + // The server-name check alone passes here: by the time the rejection + // lands, alpha is selected again. Only retiring the attempt token on + // every switch catches it. + h.ctrl.status = "connected"; + let rejectDisconnect: (err: Error) => void = () => {}; + h.disconnect.mockImplementationOnce( + () => + new Promise((_resolve, reject) => { + rejectDisconnect = reject; + }), + ); + const r = await mount(stdioServer()); + r.stdin.write("d"); // alpha's disconnect starts, and hangs + await tick(); + await press(r, [DOWN]); // alpha -> beta + await expectFrame(r, "b.js"); + await press(r, [UP]); // beta -> alpha again + await expectFrame(r, "s.js"); + rejectDisconnect(new Error("round-trip-failure")); + await waitUntil(() => (r.lastFrame() ?? "").includes("round-trip-failure")); + expect(r.lastFrame() ?? "").not.toContain("round-trip-failure"); + }); + it("switches tabs via accelerator keys", async () => { const r = await mount(stdioServer()); await press(r, ["t"]); // tools tab (server is auto-selected) diff --git a/clients/tui/__tests__/ResourceTestModal.test.tsx b/clients/tui/__tests__/ResourceTestModal.test.tsx index d9de004d..525051a0 100644 --- a/clients/tui/__tests__/ResourceTestModal.test.tsx +++ b/clients/tui/__tests__/ResourceTestModal.test.tsx @@ -77,6 +77,73 @@ const renderAndSubmit = async ( }; describe("ResourceTestModal", () => { + // RFC 6570 keeps a required expression satisfied by any ONE of its names, so + // `{a,b}` cannot be expressed with ink-form's per-field `required` flag and + // its members are left optional there. Without the modal's own group check + // the TUI would submit blank -- dropping the expression and reading a + // different resource -- while the web panel blocks the same request (#1919). + describe("required-group validation", () => { + it("refuses to submit when no name in a required group has a value", async () => { + const read = vi.fn(); + const { onClose, unmount } = await renderAndSubmit( + fakeClient(read), + makeTemplate({ uriTemplate: "x://{a,b}" }), + { a: "", b: "" }, + ); + expect(read).not.toHaveBeenCalled(); + expect(onClose).not.toHaveBeenCalled(); + unmount(); + }); + + it("blocks a required variable whose name collides with Object.prototype", async () => { + // `constructor` is a valid RFC 6570 varname, and the modal's own filter + // used to read `Object` (length 1) as a filled value. The *message* built + // from that filter came out naming no field; since frames render empty + // here (see the header note), that half is asserted on + // `unmetRequiredGroups` in the core suite. This asserts the gate. + const read = vi.fn(); + const { onClose, unmount } = await renderAndSubmit( + fakeClient(read), + makeTemplate({ uriTemplate: "x://{constructor}" }), + { constructor: "" }, + ); + expect(read).not.toHaveBeenCalled(); + expect(onClose).not.toHaveBeenCalled(); + unmount(); + }); + + it("submits once any one name in the group has a value", async () => { + const read = vi.fn().mockResolvedValue({ + result: { contents: [] }, + expandedUri: "x://only-a", + }); + const { unmount } = await renderAndSubmit( + fakeClient(read), + makeTemplate({ uriTemplate: "x://{a,b}" }), + { a: "only-a", b: "" }, + ); + // The untouched field is dropped before the read: `""` is a *defined* + // RFC 6570 value that would expand to a valueless pair, and ink-form + // cannot distinguish "never touched" from "deliberately empty". + expect(read).toHaveBeenCalledWith("x://{a,b}", { a: "only-a" }); + unmount(); + }); + + it("does not block a template whose only expression is omittable", async () => { + const read = vi.fn().mockResolvedValue({ + result: { contents: [] }, + expandedUri: "x://events", + }); + const { unmount } = await renderAndSubmit( + fakeClient(read), + makeTemplate({ uriTemplate: "x://events{?topic}" }), + { topic: "" }, + ); + expect(read).toHaveBeenCalled(); + unmount(); + }); + }); + it("renders the form initially without invoking the client", async () => { const read = vi.fn(); const api = render( 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/__tests__/tsupConfig.test.ts b/clients/tui/__tests__/tsupConfig.test.ts index a9bfc35f..8c61e9e9 100644 --- a/clients/tui/__tests__/tsupConfig.test.ts +++ b/clients/tui/__tests__/tsupConfig.test.ts @@ -207,7 +207,9 @@ describe("tui tsup config", () => { // the double cast is bridging a real structural gap, not hiding a mismatch. // A hook the patch called but the stub lacks fails as undefined here rather // than passing silently. - inkFormLabelPatch.setup( + // esbuild types `setup` as `void | Promise`; await it so the + // assertions below can't run before an async setup has registered. + await inkFormLabelPatch.setup( build as unknown as Parameters[0], ); diff --git a/clients/tui/__tests__/uriTemplateToForm.test.ts b/clients/tui/__tests__/uriTemplateToForm.test.ts index 1c522c65..c44b6b37 100644 --- a/clients/tui/__tests__/uriTemplateToForm.test.ts +++ b/clients/tui/__tests__/uriTemplateToForm.test.ts @@ -11,7 +11,38 @@ describe("uriTemplateToForm", () => { expect(form.title).toBe("Read Resource: file"); const fields = form.sections[0]!.fields; expect(fields.map((f) => f.name)).toEqual(["path", "name"]); - expect(fields[0]).toMatchObject({ type: "string", required: false }); + // Simple `{path}` sits mid-URI, so omitting it would leave an empty path + // segment rather than a shorter URI -- it is mandatory, matching the web + // panel. Both clients read this from core's `requiredGroups`. + expect(fields[0]).toMatchObject({ type: "string", required: true }); + }); + + it("names fields as the expander looks them up, not as the SDK parses them", () => { + // The SDK reports these as ";id" and "id:3"; submitting under those keys + // would make the expander find nothing and drop the expression (#1919). + expect( + uriTemplateToForm("x://a{;id}", "matrix").sections[0]!.fields.map( + (f) => f.name, + ), + ).toEqual(["id"]); + expect( + uriTemplateToForm("x://a/{id:3}", "prefix").sections[0]!.fields.map( + (f) => f.name, + ), + ).toEqual(["id"]); + }); + + it("leaves a shared required group optional rather than demanding every name", () => { + // `{a,b}` is satisfied by either name, and ink-form cannot say "any one + // of"; marking both required would refuse input the expander accepts. + const fields = uriTemplateToForm("x://{a,b}", "pair").sections[0]!.fields; + expect(fields.map((f) => f.name)).toEqual(["a", "b"]); + expect(fields.every((f) => f.required === false)).toBe(true); + }); + + it("marks an omittable variable optional", () => { + const fields = uriTemplateToForm("x://a{?topic}", "q").sections[0]!.fields; + expect(fields[0]).toMatchObject({ name: "topic", required: false }); }); it("returns an empty Template Variables section for a static URI", () => { @@ -33,3 +64,15 @@ describe("uriTemplateToForm", () => { expect(form.sections[0]!.fields).toEqual([]); }); }); + +describe("a name repeated inside one expression", () => { + it("is required, not treated as a shared group", () => { + // `{a,a}` is one requirement named twice. Before core deduplicated the + // group, `requiredGroups` returned ["a","a"], so this form's + // `length === 1` test left the field optional while ResourceTestModal's + // submit guard still refused a blank -- an un-submittable form. + const [field] = uriTemplateToForm("x://{a,a}", "T").sections[0].fields; + expect(field.name).toBe("a"); + expect(field.required).toBe(true); + }); +}); diff --git a/clients/tui/eslint.config.js b/clients/tui/eslint.config.js index b4f8a251..d37c6b7d 100644 --- a/clients/tui/eslint.config.js +++ b/clients/tui/eslint.config.js @@ -30,4 +30,19 @@ export default defineConfig([ "react-hooks/exhaustive-deps": "warn", }, }, + { + // Type-aware pass for `no-floating-promises` (#1959) — see the CLI config + // for the reasoning. Both tsconfig projects are listed because `src` and + // `__tests__` live in different ones. + files: ["**/*.{ts,tsx}"], + languageOptions: { + parserOptions: { + project: ["./tsconfig.json", "./tsconfig.test.json"], + tsconfigRootDir: import.meta.dirname, + }, + }, + rules: { + "@typescript-eslint/no-floating-promises": "error", + }, + }, ]); diff --git a/clients/tui/package-lock.json b/clients/tui/package-lock.json index 062b7e2b..9261589c 100644 --- a/clients/tui/package-lock.json +++ b/clients/tui/package-lock.json @@ -4295,9 +4295,9 @@ } }, "node_modules/nanoid": { - "version": "3.3.16", - "resolved": "https://registry.npmjs.org/nanoid/-/nanoid-3.3.16.tgz", - "integrity": "sha512-bzlKTyNJ7+LdGIIwy8ijFpIqEQIvafahV7eYykJ8Cvh42EdJeODoJ6gUJXpQJvej1BddH8OqTXZNE/KfbWAu8Q==", + "version": "3.3.18", + "resolved": "https://registry.npmjs.org/nanoid/-/nanoid-3.3.18.tgz", + "integrity": "sha512-DTg4MJbGMWkfi6VZFdNt2/caMbQy4Ou+Op/hJQvGEWcnVfoA1QA+xzRKAzw9jD6+GVOOeYr/mIcuDSdug6F6+w==", "dev": true, "funding": [ { diff --git a/clients/tui/src/App.tsx b/clients/tui/src/App.tsx index 62c9e743..240bbd82 100644 --- a/clients/tui/src/App.tsx +++ b/clients/tui/src/App.tsx @@ -170,6 +170,17 @@ function App({ pendingStepUpRef.current = pendingStepUp; }, [pendingStepUp]); const [connectError, setConnectError] = useState(null); + // Monotonic token for in-flight disconnects — see handleDisconnect. + const disconnectAttemptRef = useRef(0); + // A failed disconnect is deliberately NOT folded into `connectError`. That + // one is only rendered by InfoTab when the status is "error", which a + // rejected disconnect leaves untouched (the status stays "connected"), so + // the message would never be seen — and it feeds + // `connectError ?? inspectorLastError`, where a stale value would go on + // masking a later, real connection error. This renders in the header + // regardless of status and is cleared on the next connect/disconnect + // attempt and on a server switch. + const [disconnectError, setDisconnectError] = useState(null); const oauthInProgressRef = useRef(false); const callbackServerRef = useRef(null); const selectedServerRef = useRef(null); @@ -434,6 +445,13 @@ function App({ useEffect(() => { setOauthStatus("idle"); setOauthMessage(null); + // The header banner is server-scoped, so a failure from the server we just + // left must not be read as this one's. Clearing is not enough: a switch + // away and back (A → B → A) would leave both the attempt token and the + // server name matching again, so the stale rejection would land on the + // re-selected A. Retiring the token on every switch is what closes that. + disconnectAttemptRef.current++; + setDisconnectError(null); const stepUp = pendingStepUpRef.current; if (stepUp && selectedServer && stepUp.serverName !== selectedServer) { setPendingStepUp(null); @@ -757,6 +775,10 @@ function App({ if (!selectedServer || !selectedInspectorClient || !selectedServerConfig) { return; } + // A connect attempt supersedes whatever the last disconnect reported — + // including one still in flight, which is what the counter bump retires. + disconnectAttemptRef.current++; + setDisconnectError(null); const finishConnect = async () => { await connectInspector(); @@ -887,8 +909,31 @@ function App({ // Disconnect handler const handleDisconnect = useCallback(async () => { if (!selectedServer) return; - await disconnectInspector(); - // InspectorClient will update status automatically, and data is preserved + // Claim this attempt before awaiting. A disconnect can still be in flight + // when the user switches servers or presses 'd' again, and it is the + // *stale* one that usually rejects — so the catch below must be able to + // tell "my failure" from "one that has since been superseded", or server + // A's error lands in server B's header. `handleConnect` bumps the same + // counter for the same reason. + const attempt = ++disconnectAttemptRef.current; + const attemptServer = selectedServer; + // Clear first, so a retry that succeeds leaves no stale message behind. + setDisconnectError(null); + try { + await disconnectInspector(); + // InspectorClient will update status automatically, and data is preserved + } catch (err) { + // Nothing above this catches: the only caller is the key handler, which + // cannot await, so without this the rejection escapes unhandled. A + // superseded one is still owned here — just not published. + if ( + disconnectAttemptRef.current !== attempt || + selectedServerRef.current !== attemptServer + ) { + return; + } + setDisconnectError(err instanceof Error ? err.message : String(err)); + } }, [selectedServer, disconnectInspector]); const handleClearOAuth = useCallback(async () => { @@ -1428,12 +1473,15 @@ function App({ input.toLowerCase() === "c" && (inspectorStatus === "disconnected" || inspectorStatus === "error") ) { - handleConnect(); + // Both handlers own their failures internally (each ends in a catch + // that surfaces the message), so there is nothing left for a key + // handler — which cannot await — to do with the promise. + void handleConnect(); } else if ( input.toLowerCase() === "d" && (inspectorStatus === "connected" || inspectorStatus === "connecting") ) { - handleDisconnect(); + void handleDisconnect(); } } }); @@ -1617,6 +1665,11 @@ function App({ OAuth: {oauthMessage} )} + {disconnectError && ( + + Disconnect failed: {disconnectError} + + )} diff --git a/clients/tui/src/components/PromptsTab.tsx b/clients/tui/src/components/PromptsTab.tsx index 4f90c1a7..46db93b5 100644 --- a/clients/tui/src/components/PromptsTab.tsx +++ b/clients/tui/src/components/PromptsTab.tsx @@ -53,8 +53,10 @@ export function PromptsTab({ if (selectedPrompt.arguments && selectedPrompt.arguments.length > 0) { onFetchPrompt(selectedPrompt); } else { - // No arguments, fetch directly - (async () => { + // No arguments, fetch directly. The IIFE catches everything it can + // throw, so there is no rejection for this key handler — which + // cannot await — to own. + void (async () => { try { const invocation = await inspectorClient.getPrompt( selectedPrompt.name, diff --git a/clients/tui/src/components/ResourceTestModal.tsx b/clients/tui/src/components/ResourceTestModal.tsx index 914b4bca..87c3ddeb 100644 --- a/clients/tui/src/components/ResourceTestModal.tsx +++ b/clients/tui/src/components/ResourceTestModal.tsx @@ -5,6 +5,11 @@ import { InspectorClient } from "@inspector/core/mcp/index.js"; import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js"; import type { ReadResourceResult } from "@modelcontextprotocol/client"; import { uriTemplateToForm } from "../utils/uriTemplateToForm.js"; +import { + definedValues, + requiredGroups, + unmetRequiredGroups, +} from "@inspector/core/mcp/uriTemplate.js"; import { ScrollView, type ScrollViewRef } from "ink-scroll-view"; // Helper to extract error message from various error types @@ -133,14 +138,45 @@ export function ResourceTestModal({ const handleFormSubmit = async (values: Record) => { if (!inspectorClient || !template) return; + // RFC 6570 keeps a required expression satisfied by any ONE of its names, + // so `{a,b}` cannot be expressed with ink-form's per-field `required` flag + // and its members are left optional there. Without this check the TUI would + // submit `{a,b}` completely blank -- dropping the expression and reading a + // different resource -- while the web panel blocks the same request (#1919). + // The message and the gate come from the SAME pass. Filtering the groups + // again here with a bare `values[name]` disagreed with the gate for a + // variable named `constructor` or `toString`: the inherited member read as + // filled, so the list came out empty and the error named no field at all. + const unmetGroups = unmetRequiredGroups( + requiredGroups(template.uriTemplate), + values, + ); + if (unmetGroups.length > 0) { + const unmet = unmetGroups.map((names) => names.join(" or ")); + setResult({ + input: values, + output: null, + error: `Missing required template variable(s): ${unmet.join(", ")}`, + duration: 0, + uri: template.uriTemplate, + }); + setState("results"); + return; + } + setState("loading"); const startTime = Date.now(); try { // Use InspectorClient's readResourceFromTemplate method which encapsulates template expansion and resource reading + // Blanks are dropped HERE, not in the expander: a key present with `""` + // is a defined RFC 6570 value that legitimately expands to `?topic=`, + // but ink-form hands back `""` for every field the user never touched, + // so this form cannot tell the two apart. The web panel does the same at + // its own boundary. const invocation = await inspectorClient.readResourceFromTemplate( template.uriTemplate, - values, + definedValues(values), ); const duration = Date.now() - startTime; diff --git a/clients/tui/src/components/ResourcesTab.tsx b/clients/tui/src/components/ResourcesTab.tsx index 86ba91ac..4d0fa05d 100644 --- a/clients/tui/src/components/ResourcesTab.tsx +++ b/clients/tui/src/components/ResourcesTab.tsx @@ -186,7 +186,9 @@ export function ResourcesTab({ } }; - fetchContent(); + // An effect body is synchronous, and fetchContent already catches + // everything it can throw, so there is no rejection left to own. + void fetchContent(); }, [shouldFetchResource, inspectorClient, onAuthRecoveryRequired]); const listWidth = Math.floor(width * 0.4); 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/tui/src/utils/uriTemplateToForm.ts b/clients/tui/src/utils/uriTemplateToForm.ts index c8a027e9..af580a98 100644 --- a/clients/tui/src/utils/uriTemplateToForm.ts +++ b/clients/tui/src/utils/uriTemplateToForm.ts @@ -4,32 +4,50 @@ import type { FormStructure, FormSection, FormField } from "ink-form"; import { UriTemplate } from "@modelcontextprotocol/client"; +import { + requiredGroups, + templateVariables, +} from "@inspector/core/mcp/uriTemplate.js"; /** - * Converts a URI Template to ink-form structure + * Converts a URI Template to ink-form structure. + * + * Fields come from core's `templateVariables`, the same parser + * `InspectorClient.readResourceFromTemplate` expands through, so the key this + * form submits is the key the expander looks up. Using the SDK's + * `variableNames` here instead is not merely untidy -- it mangles the name: + * `{;id}` yields `";id"` and `{id:3}` yields `"id:3"`, so the form would submit + * `{ ";id": "7" }` while the expander looks for `id`, silently dropping the + * value and the whole expression with it (#1919). + * + * The SDK template is still constructed, but only to validate: core's parser is + * deliberately lenient (an unclosed `{` becomes literal text), so this is what + * still surfaces a malformed template as an empty form plus a logged error. */ export function uriTemplateToForm( uriTemplate: string, templateName: string, ): FormStructure { - const fields: FormField[] = []; + let fields: FormField[] = []; try { - const template = new UriTemplate(uriTemplate); - /* v8 ignore next -- UriTemplate.variableNames is a getter that always - returns a string[]; the `|| []` fallback is an unreachable guard. */ - const variableNames = template.variableNames || []; - - for (const variableName of variableNames) { - const field: FormField = { - name: variableName, - label: variableName, - type: "string", - required: false, // URI template variables are typically optional - }; - - fields.push(field); - } + new UriTemplate(uriTemplate); + // Only a variable that is the *sole* member of a non-omittable expression + // is genuinely mandatory. RFC 6570 drops undefined names from a multi-name + // expression, so `{a,b}` needs only one of the two -- ink-form cannot + // express "any one of these", and marking both required would refuse input + // the expander accepts. + const mandatory = new Set( + requiredGroups(uriTemplate) + .filter((names) => names.length === 1) + .map(([name]) => name), + ); + fields = templateVariables(uriTemplate).map(({ name }) => ({ + name, + label: name, + type: "string", + required: mandatory.has(name), + })); } catch (error) { // If parsing fails, return empty form console.error("Failed to parse URI template:", error); 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/eslint.config.js b/clients/web/eslint.config.js index 86602ed3..bdc636bb 100644 --- a/clients/web/eslint.config.js +++ b/clients/web/eslint.config.js @@ -9,7 +9,12 @@ import tseslint from "typescript-eslint"; import { defineConfig, globalIgnores } from "eslint/config"; export default defineConfig([ - globalIgnores(["dist", "storybook-static", "coverage"]), + // Generated output, none of it first-party source. Web is the only client + // that emits *two* bundles — `dist` (the Vite SPA) and `build` (the tsup + // prod-server runner, which vendors ~1.2MB of `undici`) — and only the first + // was originally listed, so `lint` reported a warning from inside undici's + // own source that nobody could act on (#2043). + globalIgnores(["build", "dist", "storybook-static", "coverage"]), { files: ["**/*.{ts,tsx}"], extends: [ @@ -31,5 +36,32 @@ export default defineConfig([ "react-refresh/only-export-components": "off", }, }, + { + // Type-aware pass for `no-floating-promises` (#1959). The rule needs type + // information, which `tseslint.configs.recommended` does not provide, and + // the parser needs a project that literally contains the linted file — so + // all four leaf projects are listed (`tsconfig.json` itself is a + // solution file with `files: []`, so it contains nothing). + // + // `.d.ts` is excluded: `vitest.shims.d.ts` is pulled in through a `types` + // reference rather than an `include`, so no project owns it, and a + // declaration file cannot float a promise anyway. + files: ["**/*.{ts,tsx}"], + ignores: ["**/*.d.ts"], + languageOptions: { + parserOptions: { + project: [ + "./tsconfig.app.json", + "./tsconfig.node.json", + "./tsconfig.storybook.json", + "./tsconfig.test.json", + ], + tsconfigRootDir: import.meta.dirname, + }, + }, + rules: { + "@typescript-eslint/no-floating-promises": "error", + }, + }, ...storybook.configs["flat/recommended"], ]); diff --git a/clients/web/package-lock.json b/clients/web/package-lock.json index 26f91ccf..b92eff57 100644 --- a/clients/web/package-lock.json +++ b/clients/web/package-lock.json @@ -7626,9 +7626,9 @@ } }, "node_modules/nanoid": { - "version": "3.3.16", - "resolved": "https://registry.npmjs.org/nanoid/-/nanoid-3.3.16.tgz", - "integrity": "sha512-bzlKTyNJ7+LdGIIwy8ijFpIqEQIvafahV7eYykJ8Cvh42EdJeODoJ6gUJXpQJvej1BddH8OqTXZNE/KfbWAu8Q==", + "version": "3.3.18", + "resolved": "https://registry.npmjs.org/nanoid/-/nanoid-3.3.18.tgz", + "integrity": "sha512-DTg4MJbGMWkfi6VZFdNt2/caMbQy4Ou+Op/hJQvGEWcnVfoA1QA+xzRKAzw9jD6+GVOOeYr/mIcuDSdug6F6+w==", "dev": true, "funding": [ { 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/server/server.ts b/clients/web/server/server.ts index 9f6ff134..fe465530 100644 --- a/clients/web/server/server.ts +++ b/clients/web/server/server.ts @@ -9,7 +9,6 @@ import { readFileSync } from "node:fs"; import { join, dirname } from "node:path"; import { fileURLToPath } from "node:url"; import { randomBytes } from "node:crypto"; -import open from "open"; import { serve } from "@hono/node-server"; import { serveStatic } from "@hono/node-server/serve-static"; import { Hono } from "hono"; @@ -22,6 +21,7 @@ import type { WebServerConfig } from "./web-server-config.js"; import { webServerConfigToInitialPayload, printServerBanner, + openBrowser, } from "./web-server-config.js"; import type { WebServerHandle } from "./types.js"; @@ -125,7 +125,9 @@ export async function startHonoServer( sandboxUrl ?? undefined, ); if (config.autoOpen) { - open(url); + // Never a bare `open(url)`: this callback is synchronous, so a + // rejection would escape it. See openBrowser. + openBrowser(url); } }, ); diff --git a/clients/web/server/vite-hono-plugin.ts b/clients/web/server/vite-hono-plugin.ts index 1331930c..880913e6 100644 --- a/clients/web/server/vite-hono-plugin.ts +++ b/clients/web/server/vite-hono-plugin.ts @@ -9,7 +9,6 @@ import type { IncomingMessage, ServerResponse } from "node:http"; import type { Plugin } from "vite"; -import open from "open"; // Direct leaf import — the `core/mcp/remote/node/index.ts` barrel re-exports // from `../constants.js`, which would otherwise drag the broader // `core/mcp/remote/index.ts` barrel (including `createRemoteLogger`, with its @@ -22,6 +21,7 @@ import type { WebServerConfig } from "./web-server-config.js"; import { webServerConfigToInitialPayload, printServerBanner, + openBrowser, } from "./web-server-config.js"; export function honoMiddlewarePlugin(config: WebServerConfig): Plugin { @@ -114,7 +114,9 @@ export function honoMiddlewarePlugin(config: WebServerConfig): Plugin { ); if (config.autoOpen) { - open(url); + // Never a bare `open(url)`: `logBanner` is synchronous, so a + // rejection would escape it. See openBrowser. + openBrowser(url); } }; diff --git a/clients/web/server/web-server-config.ts b/clients/web/server/web-server-config.ts index 6e77bd28..6bb13be2 100644 --- a/clients/web/server/web-server-config.ts +++ b/clients/web/server/web-server-config.ts @@ -2,6 +2,7 @@ * Config object for the web server (dev and prod). Passed in-process; no env handoff. */ +import open from "open"; import pino from "pino"; import type { Logger } from "pino"; import type { MCPConfig, MCPServerConfig } from "../../../core/mcp/types.ts"; @@ -203,6 +204,24 @@ export function printServerBanner( return url; } +/** + * Launch the user's browser at `url`, reporting a failure rather than throwing. + * + * Both callers — the prod Hono server's `serve()` listen callback and the dev + * plugin's `logBanner` — are synchronous, so a rejection has nowhere to go and + * escapes as an unhandled rejection (#1959). On a headless or otherwise + * browser-less host that rejection is the ordinary outcome, not an edge case, + * and by the time it happens the server is already listening and + * `printServerBanner` has printed a perfectly usable URL. So warn and carry + * on. Shared between the two servers deliberately: two copies of a one-line + * `.catch` is two chances for one of them to drift back to a bare `open(url)`. + */ +export function openBrowser(url: string): void { + open(url).catch((err: unknown) => { + console.warn("Could not open a browser automatically:", err); + }); +} + export interface BuildWebServerConfigOptions { initialMcpConfig?: MCPServerConfig | null; /** Catalog/session file the backend should serve; see {@link WebServerConfig.mcpConfigPath}. */ diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index 0869d0d5..fdf01413 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -363,7 +363,7 @@ vi.mock("./components/views/InspectorView/InspectorView", () => ({ InspectorView: (props: { toolCallState?: { status?: string }; toolsUi?: { - selectedToolName?: string; + selectedToolKey?: string; formValues: Record; search: string; }; @@ -384,7 +384,7 @@ vi.mock("./components/views/InspectorView/InspectorView", () => ({ onConnectionInfo: () => void; onToggleConnection: (id: string) => void; onToolsUiChange: (next: { - selectedToolName?: string; + selectedToolKey?: string; formValues: Record; search: string; }) => void; @@ -433,7 +433,7 @@ vi.mock("./components/views/InspectorView/InspectorView", () => ({ {Object.keys(props.progressByTaskId ?? {}).join(",") || "none"} - {props.toolsUi?.selectedToolName ?? "none"} + {props.toolsUi?.selectedToolKey ?? "none"} {props.toolsUi?.search || "none"} @@ -469,7 +469,7 @@ vi.mock("./components/views/InspectorView/InspectorView", () => ({ formValues: {}, search: "", ...props.toolsUi, - selectedToolName: "get_acts", + selectedToolKey: "0:get_acts", }) } > @@ -481,7 +481,7 @@ vi.mock("./components/views/InspectorView/InspectorView", () => ({ formValues: {}, search: "", ...props.toolsUi, - selectedToolName: "other_tool", + selectedToolKey: "1:other_tool", }) } > @@ -579,6 +579,8 @@ import { import * as McpIndex from "@inspector/core/mcp/index.js"; import * as FetchLogModule from "@inspector/core/mcp/state/fetchRequestLogState.js"; import { useManagedRequestorTasks } from "@inspector/core/react/useManagedRequestorTasks.js"; +import { useManagedTools } from "@inspector/core/react/useManagedTools.js"; +import { usePagedTools } from "@inspector/core/react/usePagedTools.js"; import { useMessageLog } from "@inspector/core/react/useMessageLog.js"; import { useInspectorClient } from "@inspector/core/react/useInspectorClient.js"; import { useSettingsDraft } from "@inspector/core/react/useSettingsDraft.js"; @@ -598,6 +600,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), @@ -868,7 +871,7 @@ describe("App session-scoped state reset on disconnect", () => { expect(screen.getByTestId("tool-status")).toHaveTextContent("ok"), ); - // A search keystroke leaves `selectedToolName` unchanged, so the result + // A search keystroke leaves `selectedToolKey` unchanged, so the result // stays put. await user.click(screen.getByText("set-tool-search")); await waitFor(() => @@ -2421,3 +2424,156 @@ describe("App paginated list pagination toggle (#1721)", () => { expect(lastPush?.paginatedLists).toBeFalsy(); }); }); + +// A background command runs through `runCommandInBackground`, which owns the +// rejection. Before #2049 the 17 `void runWithCommandAuthRecovery(...)` sites +// had no handler, so any non-auth failure of the wrapped operation became an +// unhandled rejection in the browser — invisible to the type-aware +// no-floating-promises rule (`void` is its sanctioned suppression) and fatal to +// `smoke:web:browser`, which hard-fails on exactly that signal. +describe("App background command rejections (#2049)", () => { + const LIST_FAILURE = new Error("list boom"); + + /** + * Record process-level unhandled rejections for the duration of a test. + * Asserting on the captured list — rather than relying on vitest failing the + * whole run — is what attributes a regression to this test rather than to + * whichever file happened to be running when the rejection surfaced. + */ + const captureUnhandledRejections = () => { + const seen: unknown[] = []; + const onUnhandled = (reason: unknown) => { + seen.push(reason); + }; + process.on("unhandledRejection", onUnhandled); + return { + seen, + stop: () => { + process.off("unhandledRejection", onUnhandled); + }, + }; + }; + + // Node reports an unhandled rejection at the end of the turn, so drain the + // macrotask queue before asserting that none was reported. + const settleRejections = async () => { + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + }; + + beforeEach(() => { + clientInstances.length = 0; + notificationsMock.show.mockClear(); + }); + + afterEach(() => { + // Restore the module defaults the other blocks render against. + vi.mocked(useManagedTools).mockReturnValue({ + tools: [{ name: "get_acts", inputSchema: { type: "object" } }], + error: null, + listChanged: false, + refresh: vi.fn().mockResolvedValue([]), + clearListChanged: clearToolsListChangedSpy, + }); + vi.mocked(usePagedTools).mockReturnValue({ + tools: [], + nextCursor: undefined, + pageCount: 0, + error: null, + loadPage: vi.fn(() => + Promise.resolve({ tools: [], nextCursor: undefined }), + ), + clear: vi.fn(), + }); + }); + + const connect = async () => { + const user = userEvent.setup(); + renderWithMantine(); + await user.click(screen.getByText("connect")); + await waitFor(() => expect(clientInstances).toHaveLength(1)); + return user; + }; + + it("does not leak an unhandled rejection when an all-pages Refresh fails", async () => { + vi.mocked(useManagedTools).mockReturnValue({ + tools: [], + error: LIST_FAILURE, + listChanged: false, + refresh: vi.fn().mockRejectedValue(LIST_FAILURE), + clearListChanged: clearToolsListChangedSpy, + }); + const rejections = captureUnhandledRejections(); + try { + const user = await connect(); + await user.click(screen.getByText("refresh-tools")); + await settleRejections(); + expect(rejections.seen).toEqual([]); + } finally { + rejections.stop(); + } + // The panel already renders the failure with a Retry, so no toast is added + // on top of it — that is the whole reason this site swallows the rejection. + expect(notificationsMock.show).not.toHaveBeenCalled(); + }); + + it("does not leak an unhandled rejection when a paginated Refresh or Load-next-page fails", async () => { + vi.mocked(usePagedTools).mockReturnValue({ + tools: [], + // A cursor is what makes Load-next-page actually fetch. + nextCursor: "cursor-1", + pageCount: 1, + error: LIST_FAILURE, + loadPage: vi.fn().mockRejectedValue(LIST_FAILURE), + clear: vi.fn(), + }); + const rejections = captureUnhandledRejections(); + try { + const user = await connect(); + // Flipping the mode drives its own page-1 load, which fails too. + await user.click(screen.getByText("paginated-on")); + await waitFor(() => + expect(screen.getByTestId("tools-paginated")).toHaveTextContent("true"), + ); + await user.click(screen.getByText("refresh-tools")); + await user.click(screen.getByText("load-more-tools")); + await settleRejections(); + expect(rejections.seen).toEqual([]); + } finally { + rejections.stop(); + } + }); + + it("toasts a set-log-level failure instead of leaking it", async () => { + const rejections = captureUnhandledRejections(); + try { + const user = await connect(); + // Single assertion: the fake client IS an EventTarget, so narrowing it + // to that intersection needs no `as unknown as` detour. + const client = clientInstances[0] as EventTarget & { + setLoggingLevel: ReturnType; + }; + client.setLoggingLevel.mockRejectedValueOnce(new Error("no logging")); + + await user.click(screen.getByText("set-level")); + + await waitFor(() => + expect(notificationsMock.show).toHaveBeenCalledWith( + expect.objectContaining({ + title: "Failed to set log level", + message: "no logging", + color: "red", + }), + ), + ); + await settleRejections(); + expect(rejections.seen).toEqual([]); + } finally { + rejections.stop(); + } + }); +}); diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 509fc079..db29e06b 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -63,6 +63,8 @@ import { ManagedRequestorTasksState } from "@inspector/core/mcp/state/managedReq import { ResourceSubscriptionsState } from "@inspector/core/mcp/state/resourceSubscriptionsState.js"; import { cleanRoots, + oauthAuthorizationParamsFromSettings, + oauthEndpointOverridesFromSettings, serializeMcpConfig, } from "@inspector/core/mcp/serverList.js"; import type { ClientConfig } from "@inspector/core/client/types.js"; @@ -163,6 +165,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 +993,7 @@ function App() { protocolEra, discoverResult, excludedTools, + malformedListItems, lastError, } = useInspectorClient(inspectorClient); const { @@ -1027,18 +1031,21 @@ function App() { tools: pagedTools, nextCursor: pagedToolsCursor, pageCount: pagedToolsPageCount, + error: pagedToolsLoadError, loadPage: loadToolsPage, } = usePagedTools(inspectorClient, pagedToolsState); const { prompts: pagedPrompts, nextCursor: pagedPromptsCursor, pageCount: pagedPromptsPageCount, + error: pagedPromptsLoadError, loadPage: loadPromptsPage, } = usePagedPrompts(inspectorClient, pagedPromptsState); const { resources: pagedResources, nextCursor: pagedResourcesCursor, pageCount: pagedResourcesPageCount, + error: pagedResourcesLoadError, loadPage: loadResourcesPage, } = usePagedResources(inspectorClient, pagedResourcesState); // The active server's persisted paginated setting drives the display mode. @@ -1055,15 +1062,25 @@ 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, paginated: paginatedLists, managedItems: managedTools, managedRefresh: refreshTools, + managedError: toolsLoadError, pagedItems: pagedTools, pagedNextCursor: pagedToolsCursor, pagedPageCount: pagedToolsPageCount, + pagedError: pagedToolsLoadError, loadPage: loadToolsPage, }); const promptsPagination = usePaginatedList({ @@ -1071,9 +1088,11 @@ function App() { paginated: paginatedLists, managedItems: managedPrompts, managedRefresh: refreshPrompts, + managedError: promptsLoadError, pagedItems: pagedPrompts, pagedNextCursor: pagedPromptsCursor, pagedPageCount: pagedPromptsPageCount, + pagedError: pagedPromptsLoadError, loadPage: loadPromptsPage, }); const resourcesPagination = usePaginatedList({ @@ -1081,9 +1100,11 @@ function App() { paginated: paginatedLists, managedItems: managedResources, managedRefresh: refreshResources, + managedError: resourcesLoadError, pagedItems: pagedResources, pagedNextCursor: pagedResourcesCursor, pagedPageCount: pagedResourcesPageCount, + pagedError: pagedResourcesLoadError, loadPage: loadResourcesPage, }); const tools = toolsPagination.items; @@ -2045,6 +2066,46 @@ function App() { [inspectorClient, activeServerId, handleCommandScopedAuthRecovery], ); + /** + * Fire-and-forget form of {@link runWithCommandAuthRecovery}, for a command + * whose caller has nothing to await (a click handler, a mode toggle). + * + * The wrapper rethrows anything that is not an `AuthRecoveryRequiredError`, + * so a bare `void runWithCommandAuthRecovery(...)` turns every non-auth + * failure — a transport error, a rejected `tools/list` — into an unhandled + * rejection in the browser (#2049). Routing every background command through + * here is what keeps a new call site from reintroducing that gap by + * omission: there is no `void` to forget. + * + * `errorTitle` picks the reporting, and the choice is per call site: + * + * - **Omit it** when the operation's own state already renders the failure — + * the list loads, whose managed/paged stores record the error and whose + * panel shows it with a Retry. A toast there would only duplicate what the + * user is already looking at, so the rejection is swallowed deliberately. + * - **Pass one** when nothing else records the failure, and the user would + * otherwise see the command silently do nothing. + */ + const runCommandInBackground = useCallback( + ( + operation: () => Promise, + source: StepUpSource, + errorTitle?: string, + ): void => { + void runWithCommandAuthRecovery(operation, source).catch( + (err: unknown) => { + if (!errorTitle) return; + notifications.show({ + title: errorTitle, + message: err instanceof Error ? err.message : String(err), + color: "red", + }); + }, + ); + }, + [runWithCommandAuthRecovery], + ); + const resumePendingReauth = useCallback( async (pending: PendingReauth) => { if (reauthResumeInProgressRef.current) { @@ -2301,11 +2362,19 @@ function App() { .map((m) => [m.key, m.value]), ) : undefined; + const serverAuthorizationParams = savedSettings + ? oauthAuthorizationParamsFromSettings(savedSettings) + : undefined; + const serverEndpointOverrides = savedSettings + ? oauthEndpointOverridesFromSettings(savedSettings) + : undefined; const oauthFromServer = savedSettings && (savedSettings.oauthClientId || savedSettings.oauthClientSecret || savedSettings.oauthScopes || + serverAuthorizationParams || + serverEndpointOverrides || savedSettings.enterpriseManaged) ? { ...(savedSettings.oauthClientId && { @@ -2317,6 +2386,10 @@ function App() { ...(savedSettings.oauthScopes && { scope: savedSettings.oauthScopes, }), + ...(serverAuthorizationParams && { + authorizationParams: serverAuthorizationParams, + }), + ...serverEndpointOverrides, ...(savedSettings.enterpriseManaged && { enterpriseManaged: true, }), @@ -3136,17 +3209,17 @@ function App() { // drops the previous tool's result — the result panel renders `toolCallState` // regardless of selection, so without this a stale result would linger under // the newly-selected tool (which has no result of its own yet). Search and - // form edits keep `selectedToolName` unchanged, so they leave the result be. - // Depends on `selectedToolName` only (not the whole `toolsUi`), so a search + // form edits keep `selectedToolKey` unchanged, so they leave the result be. + // Depends on `selectedToolKey` only (not the whole `toolsUi`), so a search // keystroke doesn't churn the callback identity. const onToolsUiChange = useCallback( (next: ToolsUiState) => { - if (next.selectedToolName !== toolsUi.selectedToolName) { + if (next.selectedToolKey !== toolsUi.selectedToolKey) { setToolCallState(undefined); } setToolsUi(next); }, - [toolsUi.selectedToolName], + [toolsUi.selectedToolKey], ); // --- MCP Apps handlers. Unlike onCallTool (which feeds the Tools panel), @@ -3441,12 +3514,16 @@ function App() { (level: LoggingLevel) => { setCurrentLogLevel(level); if (!inspectorClient) return; - void runWithCommandAuthRecovery( + // Nothing else records a `logging/setLevel` failure, and the optimistic + // level above already moved — so report it, or the selector would sit on + // a level the server never accepted with no explanation. + runCommandInBackground( () => inspectorClient.setLoggingLevel(level), "ambient", + "Failed to set log level", ); }, - [inspectorClient, runWithCommandAuthRecovery], + [inspectorClient, runCommandInBackground], ); // Modern era (#1629): no request is sent — the client stores the level and @@ -3469,51 +3546,39 @@ function App() { // (the managed state lit it on `list_changed`; nothing else clears it in // paginated mode) (#1721). clearToolsListChanged(); - void runWithCommandAuthRecovery( - () => toolsPagination.onRefresh(), - "ambient", - ); + runCommandInBackground(() => toolsPagination.onRefresh(), "ambient"); } else { - void runWithCommandAuthRecovery(() => refreshTools(), "ambient"); + runCommandInBackground(() => refreshTools(), "ambient"); } }, [ paginatedLists, toolsPagination, refreshTools, clearToolsListChanged, - runWithCommandAuthRecovery, + runCommandInBackground, ]); const onRefreshPrompts = useCallback(() => { if (paginatedLists) { clearPromptsListChanged(); - void runWithCommandAuthRecovery( - () => promptsPagination.onRefresh(), - "ambient", - ); + runCommandInBackground(() => promptsPagination.onRefresh(), "ambient"); } else { - void runWithCommandAuthRecovery(() => refreshPrompts(), "ambient"); + runCommandInBackground(() => refreshPrompts(), "ambient"); } }, [ paginatedLists, promptsPagination, refreshPrompts, clearPromptsListChanged, - runWithCommandAuthRecovery, + runCommandInBackground, ]); const onRefreshResources = useCallback(() => { if (paginatedLists) { clearResourcesListChanged(); - void runWithCommandAuthRecovery( - () => resourcesPagination.onRefresh(), - "ambient", - ); + runCommandInBackground(() => resourcesPagination.onRefresh(), "ambient"); // Resource templates always use the managed (aggregate) path. - void runWithCommandAuthRecovery( - () => refreshResourceTemplates(), - "ambient", - ); + runCommandInBackground(() => refreshResourceTemplates(), "ambient"); } else { - void runWithCommandAuthRecovery(async () => { + runCommandInBackground(async () => { await refreshResources(); await refreshResourceTemplates(); }, "ambient"); @@ -3524,7 +3589,7 @@ function App() { refreshResources, refreshResourceTemplates, clearResourcesListChanged, - runWithCommandAuthRecovery, + runCommandInBackground, ]); // The per-list sidebar toggle edits the server-wide `paginatedLists` setting: // optimistic override for an instant flip, live push so the managed state's @@ -3548,22 +3613,13 @@ function App() { // Wrap in ambient auth recovery so a mid-session 401 triggers re-auth // rather than surfacing raw, matching the all-pages refresh path. if (value) { - void runWithCommandAuthRecovery( - () => loadToolsPage(undefined), - "ambient", - ); - void runWithCommandAuthRecovery( - () => loadPromptsPage(undefined), - "ambient", - ); - void runWithCommandAuthRecovery( - () => loadResourcesPage(undefined), - "ambient", - ); + runCommandInBackground(() => loadToolsPage(undefined), "ambient"); + runCommandInBackground(() => loadPromptsPage(undefined), "ambient"); + runCommandInBackground(() => loadResourcesPage(undefined), "ambient"); } else { - void runWithCommandAuthRecovery(() => refreshTools(), "ambient"); - void runWithCommandAuthRecovery(() => refreshPrompts(), "ambient"); - void runWithCommandAuthRecovery(() => refreshResources(), "ambient"); + runCommandInBackground(() => refreshTools(), "ambient"); + runCommandInBackground(() => refreshPrompts(), "ambient"); + runCommandInBackground(() => refreshResources(), "ambient"); } } void updateServerSettings(activeServerId, next).catch((err: unknown) => { @@ -3592,34 +3648,24 @@ function App() { refreshTools, refreshPrompts, refreshResources, - runWithCommandAuthRecovery, + runCommandInBackground, ], ); // Wrap Load-next-page in ambient auth recovery too, so a paginated // paginated fetch that hits a 401 recovers like the all-pages path (#1721). const onLoadMoreTools = useCallback( - () => - void runWithCommandAuthRecovery( - () => toolsPagination.onLoadMore(), - "ambient", - ), - [toolsPagination, runWithCommandAuthRecovery], + () => runCommandInBackground(() => toolsPagination.onLoadMore(), "ambient"), + [toolsPagination, runCommandInBackground], ); const onLoadMorePrompts = useCallback( () => - void runWithCommandAuthRecovery( - () => promptsPagination.onLoadMore(), - "ambient", - ), - [promptsPagination, runWithCommandAuthRecovery], + runCommandInBackground(() => promptsPagination.onLoadMore(), "ambient"), + [promptsPagination, runCommandInBackground], ); const onLoadMoreResources = useCallback( () => - void runWithCommandAuthRecovery( - () => resourcesPagination.onLoadMore(), - "ambient", - ), - [resourcesPagination, runWithCommandAuthRecovery], + runCommandInBackground(() => resourcesPagination.onLoadMore(), "ambient"), + [resourcesPagination, runCommandInBackground], ); const toolsPaginationControls: ListPaginationControlsProps = { paginated: toolsPagination.paginated, @@ -3643,16 +3689,12 @@ function App() { onLoadMore: onLoadMoreResources, }; const onRefreshTasks = useCallback(() => { - void runWithCommandAuthRecovery(() => refreshTasks(), "ambient")?.catch( - (err: unknown) => { - notifications.show({ - title: "Failed to refresh tasks", - message: err instanceof Error ? err.message : String(err), - color: "red", - }); - }, + runCommandInBackground( + () => refreshTasks(), + "ambient", + "Failed to refresh tasks", ); - }, [refreshTasks, runWithCommandAuthRecovery]); + }, [refreshTasks, runCommandInBackground]); const onClearLogs = useCallback(() => { if (!messageLogState) return; @@ -4376,15 +4418,18 @@ function App() { latencyMs={latencyMs} tools={tools} excludedTools={excludedTools} + malformedListItems={shownMalformedListItems} prompts={prompts} resources={resources} resourceTemplates={resourceTemplates} toolsListChanged={toolsListChanged} promptsListChanged={promptsListChanged} resourcesListChanged={resourcesListChanged} - toolsLoadError={toolsLoadError} - promptsLoadError={promptsLoadError} - resourcesLoadError={resourcesLoadError ?? resourceTemplatesLoadError} + toolsLoadError={toolsPagination.error} + promptsLoadError={promptsPagination.error} + resourcesLoadError={ + resourcesPagination.error ?? resourceTemplatesLoadError + } subscriptions={subscriptions} subscriptionStreamState={subscriptionStreamState} logs={logs} diff --git a/clients/web/src/components/elements/CategoryBadge/CategoryBadge.stories.tsx b/clients/web/src/components/elements/CategoryBadge/CategoryBadge.stories.tsx index f9118c49..efbeab28 100644 --- a/clients/web/src/components/elements/CategoryBadge/CategoryBadge.stories.tsx +++ b/clients/web/src/components/elements/CategoryBadge/CategoryBadge.stories.tsx @@ -14,7 +14,7 @@ export const Transport: Story = { args: { category: "transport" }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByText("transport")).toBeInTheDocument(); + await expect(canvas.getByText("transport")).toBeInTheDocument(); }, }; @@ -22,6 +22,6 @@ export const Auth: Story = { args: { category: "auth" }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByText("auth")).toBeInTheDocument(); + await expect(canvas.getByText("auth")).toBeInTheDocument(); }, }; 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/EmbeddableScrollArea/EmbeddableScrollArea.stories.tsx b/clients/web/src/components/elements/EmbeddableScrollArea/EmbeddableScrollArea.stories.tsx index e85f9b65..87da3cdb 100644 --- a/clients/web/src/components/elements/EmbeddableScrollArea/EmbeddableScrollArea.stories.tsx +++ b/clients/web/src/components/elements/EmbeddableScrollArea/EmbeddableScrollArea.stories.tsx @@ -24,7 +24,7 @@ export const FullSize: Story = { ), play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByText("Row 1")).toBeInTheDocument(); + await expect(canvas.getByText("Row 1")).toBeInTheDocument(); }, }; @@ -38,6 +38,6 @@ export const Embedded: Story = { ), play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByText("Row 1")).toBeInTheDocument(); + await expect(canvas.getByText("Row 1")).toBeInTheDocument(); }, }; diff --git a/clients/web/src/components/elements/ExpandToggle/ExpandToggle.stories.tsx b/clients/web/src/components/elements/ExpandToggle/ExpandToggle.stories.tsx index df4054c8..90369ab1 100644 --- a/clients/web/src/components/elements/ExpandToggle/ExpandToggle.stories.tsx +++ b/clients/web/src/components/elements/ExpandToggle/ExpandToggle.stories.tsx @@ -15,7 +15,9 @@ export const Collapsed: Story = { args: { expanded: false }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByRole("button", { name: "Expand" })).toBeInTheDocument(); + await expect( + canvas.getByRole("button", { name: "Expand" }), + ).toBeInTheDocument(); }, }; @@ -23,7 +25,7 @@ export const Expanded: Story = { args: { expanded: true }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect( + await expect( canvas.getByRole("button", { name: "Collapse" }), ).toBeInTheDocument(); }, 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/elements/McpErrorBadge/McpErrorBadge.stories.tsx b/clients/web/src/components/elements/McpErrorBadge/McpErrorBadge.stories.tsx index 4616b4fa..c189d654 100644 --- a/clients/web/src/components/elements/McpErrorBadge/McpErrorBadge.stories.tsx +++ b/clients/web/src/components/elements/McpErrorBadge/McpErrorBadge.stories.tsx @@ -14,7 +14,7 @@ export const HeaderMismatch: Story = { args: { code: -32020, name: "HeaderMismatch" }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByText("-32020 HeaderMismatch")).toBeInTheDocument(); + await expect(canvas.getByText("-32020 HeaderMismatch")).toBeInTheDocument(); }, }; @@ -22,7 +22,7 @@ export const MissingCapability: Story = { args: { code: -32021, name: "MissingRequiredClientCapability" }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect( + await expect( canvas.getByText("-32021 MissingRequiredClientCapability"), ).toBeInTheDocument(); }, @@ -32,7 +32,7 @@ export const UnsupportedVersion: Story = { args: { code: -32022, name: "UnsupportedProtocolVersion" }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect( + await expect( canvas.getByText("-32022 UnsupportedProtocolVersion"), ).toBeInTheDocument(); }, @@ -47,6 +47,6 @@ export const MethodNotFound: Story = { }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByText("-32601 MethodNotFound")).toBeInTheDocument(); + await expect(canvas.getByText("-32601 MethodNotFound")).toBeInTheDocument(); }, }; diff --git a/clients/web/src/components/elements/MessageDirectionBadge/MessageDirectionBadge.stories.tsx b/clients/web/src/components/elements/MessageDirectionBadge/MessageDirectionBadge.stories.tsx index a4aa55e0..c99b8a12 100644 --- a/clients/web/src/components/elements/MessageDirectionBadge/MessageDirectionBadge.stories.tsx +++ b/clients/web/src/components/elements/MessageDirectionBadge/MessageDirectionBadge.stories.tsx @@ -14,7 +14,7 @@ export const Outgoing: Story = { args: { direction: "outgoing" }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByText("client → server")).toBeInTheDocument(); + await expect(canvas.getByText("client → server")).toBeInTheDocument(); }, }; @@ -22,6 +22,6 @@ export const Incoming: Story = { args: { direction: "incoming" }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByText("server → client")).toBeInTheDocument(); + await expect(canvas.getByText("server → client")).toBeInTheDocument(); }, }; diff --git a/clients/web/src/components/elements/MethodBadge/MethodBadge.stories.tsx b/clients/web/src/components/elements/MethodBadge/MethodBadge.stories.tsx index 91e3abb5..312d249a 100644 --- a/clients/web/src/components/elements/MethodBadge/MethodBadge.stories.tsx +++ b/clients/web/src/components/elements/MethodBadge/MethodBadge.stories.tsx @@ -14,7 +14,7 @@ export const ToolsList: Story = { args: { method: "tools/list" }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByText("tools/list")).toBeInTheDocument(); + await expect(canvas.getByText("tools/list")).toBeInTheDocument(); }, }; @@ -22,6 +22,6 @@ export const TasksList: Story = { args: { method: "tasks/list" }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByText("tasks/list")).toBeInTheDocument(); + await expect(canvas.getByText("tasks/list")).toBeInTheDocument(); }, }; diff --git a/clients/web/src/components/elements/MonitoringToggle/MonitoringToggle.stories.tsx b/clients/web/src/components/elements/MonitoringToggle/MonitoringToggle.stories.tsx index 9541812e..109a47d8 100644 --- a/clients/web/src/components/elements/MonitoringToggle/MonitoringToggle.stories.tsx +++ b/clients/web/src/components/elements/MonitoringToggle/MonitoringToggle.stories.tsx @@ -15,7 +15,7 @@ export const Closed: Story = { args: { open: false }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect( + await expect( canvas.getByRole("button", { name: "Open monitoring sidebar" }), ).toBeInTheDocument(); }, @@ -25,7 +25,7 @@ export const Open: Story = { args: { open: true }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect( + await expect( canvas.getByRole("button", { name: "Close monitoring sidebar" }), ).toBeInTheDocument(); }, diff --git a/clients/web/src/components/elements/PinToggle/PinToggle.stories.tsx b/clients/web/src/components/elements/PinToggle/PinToggle.stories.tsx index d259eb84..a15900d4 100644 --- a/clients/web/src/components/elements/PinToggle/PinToggle.stories.tsx +++ b/clients/web/src/components/elements/PinToggle/PinToggle.stories.tsx @@ -15,7 +15,9 @@ export const Unpinned: Story = { args: { pinned: false }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByRole("button", { name: "Pin" })).toBeInTheDocument(); + await expect( + canvas.getByRole("button", { name: "Pin" }), + ).toBeInTheDocument(); }, }; @@ -23,6 +25,8 @@ export const Pinned: Story = { args: { pinned: true }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByRole("button", { name: "Unpin" })).toBeInTheDocument(); + await expect( + canvas.getByRole("button", { name: "Unpin" }), + ).toBeInTheDocument(); }, }; diff --git a/clients/web/src/components/elements/ReplayButton/ReplayButton.stories.tsx b/clients/web/src/components/elements/ReplayButton/ReplayButton.stories.tsx index 6391f2b4..a7bd9b17 100644 --- a/clients/web/src/components/elements/ReplayButton/ReplayButton.stories.tsx +++ b/clients/web/src/components/elements/ReplayButton/ReplayButton.stories.tsx @@ -14,6 +14,8 @@ type Story = StoryObj; export const Default: Story = { play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByRole("button", { name: "Replay" })).toBeInTheDocument(); + await expect( + canvas.getByRole("button", { name: "Replay" }), + ).toBeInTheDocument(); }, }; diff --git a/clients/web/src/components/elements/ResizeHandle/ResizeHandle.stories.tsx b/clients/web/src/components/elements/ResizeHandle/ResizeHandle.stories.tsx index 4fb1c122..7b8072dc 100644 --- a/clients/web/src/components/elements/ResizeHandle/ResizeHandle.stories.tsx +++ b/clients/web/src/components/elements/ResizeHandle/ResizeHandle.stories.tsx @@ -44,7 +44,7 @@ export const Default: Story = { render: () => , play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect( + await expect( canvas.getByRole("separator", { name: "Resize demo panel" }), ).toBeInTheDocument(); }, diff --git a/clients/web/src/components/elements/ScreenStage/ScreenStage.stories.tsx b/clients/web/src/components/elements/ScreenStage/ScreenStage.stories.tsx index ab215303..c7af29bc 100644 --- a/clients/web/src/components/elements/ScreenStage/ScreenStage.stories.tsx +++ b/clients/web/src/components/elements/ScreenStage/ScreenStage.stories.tsx @@ -48,7 +48,7 @@ export const FillsHostWidth: Story = { // sharper discriminator: a row flex leaves the layer full-width while // collapsing the child to the single "x"). const layer = child.parentElement as HTMLElement; - expect(child.getBoundingClientRect().width).toBeCloseTo( + await expect(child.getBoundingClientRect().width).toBeCloseTo( layer.getBoundingClientRect().width, 0, ); @@ -77,11 +77,11 @@ export const FillVariant: Story = { // Width still stretches onto the child (relative to the layer, as above); // the `fill` height comes from `bottom: 0` making the layer span the host. const layer = child.parentElement as HTMLElement; - expect(child.getBoundingClientRect().width).toBeCloseTo( + await expect(child.getBoundingClientRect().width).toBeCloseTo( layer.getBoundingClientRect().width, 0, ); - expect(layer.getBoundingClientRect().height).toBeGreaterThan(160 - 4); + await expect(layer.getBoundingClientRect().height).toBeGreaterThan(160 - 4); }, }; @@ -97,6 +97,6 @@ export const Inactive: Story = { ), play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.queryByTestId("stage-child")).not.toBeInTheDocument(); + await expect(canvas.queryByTestId("stage-child")).not.toBeInTheDocument(); }, }; 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); + const tasksRow = canvas.getByText("Tasks"); + await expect(tasksRow.previousElementSibling).toHaveTextContent("✓"); + }, }; // A modern server that omitted the optional `_meta` serverInfo stamp (#1772). @@ -106,8 +114,10 @@ export const ServerInfoNotReported: Story = { }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.queryByText("my-catalog-name")).not.toBeInTheDocument(); - expect(canvas.getAllByText(SERVER_INFO_NOT_REPORTED_LABEL)).toHaveLength(2); + await expect(canvas.queryByText("my-catalog-name")).not.toBeInTheDocument(); + await expect( + canvas.getAllByText(SERVER_INFO_NOT_REPORTED_LABEL), + ).toHaveLength(2); }, }; @@ -161,3 +171,37 @@ export const WithOAuth: Story = { }, }, }; + +// An authorization server that also returned an `id_token` (#2019). Shown for +// inspection only — decoding it is display-only, and it is never used as a +// credential. +export const WithOAuthIdToken: Story = { + args: { + initializeResult: { + protocolVersion: "2025-03-26", + serverInfo: { name: "OIDC-fronted Server", version: "3.0.0" }, + capabilities: { tools: { listChanged: true } }, + }, + clientCapabilities: { roots: { listChanged: true } }, + transport: "streamable-http", + oauth: { + protocol: "standard", + authorized: true, + authUrl: "https://auth.example.com/oauth2/authorize", + scopes: ["openid", "email", "read"], + accessToken: "opaque-access-token", + // header {"alg":"none"} / payload {"sub":"user-42","email":"a@b.test"} + idToken: + "eyJhbGciOiJub25lIn0.eyJzdWIiOiJ1c2VyLTQyIiwiZW1haWwiOiJhQGIudGVzdCJ9.", + }, + }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + await 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" }), + ); + await 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..0ebb740f 100644 --- a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx +++ b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx @@ -31,6 +31,21 @@ const fullClientCaps: ClientCapabilities = { experimental: {}, }; +// The glyphs `CapabilityItem` renders beside each capability label. +const SUPPORTED_MARK = "✓"; +const UNSUPPORTED_MARK = "✗"; + +/** + * Read the ✓/✗ a capability row is showing. The mark is the label's preceding + * sibling inside the row `Group`, so asserting on it — rather than on the + * label's mere presence — is what distinguishes "supported" from "listed". + */ +function capabilityMark(label: string): string | undefined { + return ( + screen.getByText(label).previousElementSibling?.textContent ?? undefined + ); +} + describe("ConnectionInfoContent", () => { it("renders server implementation fields under the heading", () => { renderWithMantine( @@ -181,6 +196,96 @@ describe("ConnectionInfoContent", () => { expect(screen.getByText("Sampling")).toBeInTheDocument(); }); + it("marks Tasks supported on a modern connection that advertises the tasks extension (#1887)", () => { + renderWithMantine( + , + ); + expect(capabilityMark("Tasks")).toBe(SUPPORTED_MARK); + }); + + it("still marks Tasks unsupported on a modern connection without the extension (#1887)", () => { + renderWithMantine( + , + ); + expect(capabilityMark("Tasks")).toBe(UNSUPPORTED_MARK); + }); + + it("does not read the tasks extension on a legacy connection (#1887)", () => { + renderWithMantine( + , + ); + // Legacy tasks support is `capabilities.tasks`, which this server omits — + // an extension key alone must not turn the row green. + expect(capabilityMark("Tasks")).toBe(UNSUPPORTED_MARK); + }); + + it("marks Tasks supported on a legacy connection from capabilities.tasks", () => { + renderWithMantine( + , + ); + expect(capabilityMark("Tasks")).toBe(SUPPORTED_MARK); + }); + + it("does not extension-promote a capability other than tasks (#1887)", () => { + renderWithMantine( + , + ); + expect(capabilityMark("Prompts")).toBe(UNSUPPORTED_MARK); + expect(capabilityMark("Tasks")).toBe(UNSUPPORTED_MARK); + }); + it("renders instructions when present", () => { renderWithMantine( { 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..9ee75b8a 100644 --- a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.tsx +++ b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.tsx @@ -14,8 +14,10 @@ import type { DiscoverResult, InitializeResult, ProtocolEra, + ServerCapabilities, } from "@modelcontextprotocol/client"; import type { ServerType } from "@inspector/core/mcp/types.js"; +import { TASKS_EXTENSION_KEY } from "@inspector/core/mcp/modernTaskSchemas.js"; import type { OAuthClientRegistrationKind } from "@inspector/core/auth/types.js"; import { CapabilityItem, @@ -24,7 +26,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 +36,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"; } @@ -182,16 +191,62 @@ const CLIENT_CAPABILITY_KEYS: CapabilityKey[] = [ export const CLEAR_OAUTH_STATE_AND_DISCONNECT_LABEL = "Clear OAuth state and disconnect"; +/** + * Server capabilities the modern (2026-07-28) era expresses as a negotiated + * *extension* rather than a top-level `capabilities` key. `tasks` is the only + * one today: SEP-2663 moved task support to + * `capabilities.extensions["io.modelcontextprotocol/tasks"]`, so a modern + * tasks-capable server left the Tasks row showing a red ✗ while the very same + * extension id was listed under "Server Extensions" two sections below + * (#1887). Keyed the same way `InspectorClient.isTasksExtensionNegotiated()` + * gates the Tasks tab, so the checkmark and the tab agree. + */ +const MODERN_EXTENSION_BACKED_CAPABILITIES: Partial< + Record +> = { + tasks: TASKS_EXTENSION_KEY, +}; + +function isCapabilityPresent( + capabilities: Record, + key: CapabilityKey, +): boolean { + return key in capabilities && capabilities[key] != null; +} + function getCapabilityEntries( capabilities: Record, knownKeys: CapabilityKey[], ): { capability: CapabilityKey; supported: boolean }[] { return knownKeys.map((key) => ({ capability: key, - supported: key in capabilities && capabilities[key] != null, + supported: isCapabilityPresent(capabilities, key), })); } +/** + * Server-side capability entries. Same presence rule as the client column, + * plus the modern extension fallback above — gated on the negotiated era so a + * legacy connection is still judged purely on its `initialize` capabilities. + */ +function getServerCapabilityEntries( + capabilities: ServerCapabilities, + era: ProtocolEra | undefined, +): { capability: CapabilityKey; supported: boolean }[] { + const modern = isModernEra(era); + return SERVER_CAPABILITY_KEYS.map((key) => { + const extensionKey = MODERN_EXTENSION_BACKED_CAPABILITIES[key]; + const viaExtension = + modern && + extensionKey !== undefined && + capabilities.extensions?.[extensionKey] != null; + return { + capability: key, + supported: isCapabilityPresent(capabilities, key) || viaExtension, + }; + }); +} + export function ConnectionInfoContent({ initializeResult, serverInfoReported = true, @@ -222,7 +277,7 @@ export function ConnectionInfoContent({ ? serverInfo.version?.trim() || "—" : SERVER_INFO_NOT_REPORTED_LABEL; - const serverCaps = getCapabilityEntries(capabilities, SERVER_CAPABILITY_KEYS); + const serverCaps = getServerCapabilityEntries(capabilities, protocolEra); const clientCaps = getCapabilityEntries( clientCapabilities, CLIENT_CAPABILITY_KEYS, @@ -370,20 +425,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/LogStreamPanel/LogStreamPanel.stories.tsx b/clients/web/src/components/groups/LogStreamPanel/LogStreamPanel.stories.tsx index 111f54e6..a7545dc3 100644 --- a/clients/web/src/components/groups/LogStreamPanel/LogStreamPanel.stories.tsx +++ b/clients/web/src/components/groups/LogStreamPanel/LogStreamPanel.stories.tsx @@ -77,6 +77,6 @@ export const WithEntries: Story = { }, // List scrollbar reserves a gutter and stays hidden when idle (#1474). play: async ({ canvasElement }) => { - expectScrollbarGutterIdleHidden(canvasElement); + await expectScrollbarGutterIdleHidden(canvasElement); }, }; diff --git a/clients/web/src/components/groups/MessageDirectionFilter/MessageDirectionFilter.stories.tsx b/clients/web/src/components/groups/MessageDirectionFilter/MessageDirectionFilter.stories.tsx index 362b30bd..8296dd63 100644 --- a/clients/web/src/components/groups/MessageDirectionFilter/MessageDirectionFilter.stories.tsx +++ b/clients/web/src/components/groups/MessageDirectionFilter/MessageDirectionFilter.stories.tsx @@ -31,10 +31,10 @@ export const AllVisible: Story = { args: { visibleDirections: { client: true, server: true } }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect( + await expect( canvas.getByRole("button", { name: "client → server" }), ).toBeInTheDocument(); - expect( + await expect( canvas.getByRole("button", { name: "Deselect All" }), ).toBeInTheDocument(); }, diff --git a/clients/web/src/components/groups/MonitoringControls/MonitoringControls.stories.tsx b/clients/web/src/components/groups/MonitoringControls/MonitoringControls.stories.tsx index 3a3eb7ff..eca1065a 100644 --- a/clients/web/src/components/groups/MonitoringControls/MonitoringControls.stories.tsx +++ b/clients/web/src/components/groups/MonitoringControls/MonitoringControls.stories.tsx @@ -20,7 +20,7 @@ type Story = StoryObj; export const AllTabs: Story = { play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByRole("radio", { name: "Logs" })).toBeChecked(); + await expect(canvas.getByRole("radio", { name: "Logs" })).toBeChecked(); }, }; @@ -28,6 +28,6 @@ export const StdioServer: Story = { args: { tabs: ["Logs", "Protocol"], value: "Protocol" }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.queryByRole("radio", { name: "Network" })).toBeNull(); + await expect(canvas.queryByRole("radio", { name: "Network" })).toBeNull(); }, }; diff --git a/clients/web/src/components/groups/MonitoringScreen/MonitoringScreen.stories.tsx b/clients/web/src/components/groups/MonitoringScreen/MonitoringScreen.stories.tsx index 56c1cca2..b91d5d3b 100644 --- a/clients/web/src/components/groups/MonitoringScreen/MonitoringScreen.stories.tsx +++ b/clients/web/src/components/groups/MonitoringScreen/MonitoringScreen.stories.tsx @@ -47,7 +47,7 @@ type Story = StoryObj; export const Logs: Story = { play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByText("Log stream")).toBeInTheDocument(); + await expect(canvas.getByText("Log stream")).toBeInTheDocument(); }, }; @@ -55,6 +55,6 @@ export const Protocol: Story = { args: { value: "Protocol" }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByText("Request history")).toBeInTheDocument(); + await expect(canvas.getByText("Request history")).toBeInTheDocument(); }, }; diff --git a/clients/web/src/components/groups/NetworkStreamPanel/NetworkStreamPanel.stories.tsx b/clients/web/src/components/groups/NetworkStreamPanel/NetworkStreamPanel.stories.tsx index a4248d1e..e006e261 100644 --- a/clients/web/src/components/groups/NetworkStreamPanel/NetworkStreamPanel.stories.tsx +++ b/clients/web/src/components/groups/NetworkStreamPanel/NetworkStreamPanel.stories.tsx @@ -50,7 +50,7 @@ export const WithEntries: Story = { args: { entries: sample }, // List scrollbar reserves a gutter and stays hidden when idle (#1474). play: async ({ canvasElement }) => { - expectScrollbarGutterIdleHidden(canvasElement); + await expectScrollbarGutterIdleHidden(canvasElement); }, }; diff --git a/clients/web/src/components/groups/OutputValidationModal/OutputValidationModal.stories.tsx b/clients/web/src/components/groups/OutputValidationModal/OutputValidationModal.stories.tsx index 0942c7dd..40544352 100644 --- a/clients/web/src/components/groups/OutputValidationModal/OutputValidationModal.stories.tsx +++ b/clients/web/src/components/groups/OutputValidationModal/OutputValidationModal.stories.tsx @@ -45,14 +45,14 @@ export const Default: Story = { // Mantine renders the modal in a portal at document.body. const body = within(canvasElement.ownerDocument.body); await body.findByText("Output schema validation"); - expect(body.getByText(/open_pattern_editor/)).toBeInTheDocument(); + await expect(body.getByText(/open_pattern_editor/)).toBeInTheDocument(); const details = body.getByLabelText( "Validation details", ) as HTMLTextAreaElement; - expect(details.value).toContain( + await expect(details.value).toContain( "data/samples/0 must NOT have additional properties", ); - expect(details.readOnly).toBe(true); + await expect(details.readOnly).toBe(true); }, }; diff --git a/clients/web/src/components/groups/PendingClientRequestModal/PendingClientRequestModal.stories.tsx b/clients/web/src/components/groups/PendingClientRequestModal/PendingClientRequestModal.stories.tsx index 5130e98c..ee81a687 100644 --- a/clients/web/src/components/groups/PendingClientRequestModal/PendingClientRequestModal.stories.tsx +++ b/clients/web/src/components/groups/PendingClientRequestModal/PendingClientRequestModal.stories.tsx @@ -122,11 +122,11 @@ export const Sampling: Story = { play: async ({ canvasElement, args }) => { const body = within(canvasElement.ownerDocument.body); await body.findByText("Sampling Request"); - expect( + await expect( body.getByText("The server is requesting an LLM completion."), ).toBeInTheDocument(); await userEvent.click(body.getByRole("button", { name: "Send Response" })); - expect(args.onSamplingRespond).toHaveBeenCalled(); + await expect(args.onSamplingRespond).toHaveBeenCalled(); }, }; @@ -144,8 +144,8 @@ export const SamplingInputRequired: Story = { play: async ({ canvasElement }) => { const body = within(canvasElement.ownerDocument.body); await body.findByText("Sampling Request"); - expect(body.getByText("input_required")).toBeInTheDocument(); - expect(body.getByText(/sent back as a retry/i)).toBeInTheDocument(); + await expect(body.getByText("input_required")).toBeInTheDocument(); + await expect(body.getByText(/sent back as a retry/i)).toBeInTheDocument(); }, }; @@ -172,11 +172,11 @@ export const ElicitationForm: Story = { play: async ({ canvasElement, args }) => { const body = within(canvasElement.ownerDocument.body); await body.findByText("Elicitation Request"); - expect( + await expect( body.getByText(/Please provide your database connection details/), ).toBeInTheDocument(); await userEvent.click(body.getByRole("button", { name: "Submit" })); - expect(args.onElicitationRespond).toHaveBeenCalledWith({ + await expect(args.onElicitationRespond).toHaveBeenCalledWith({ action: "accept", content: {}, }); @@ -207,7 +207,7 @@ export const ElicitationUrl: Story = { play: async ({ canvasElement }) => { const body = within(canvasElement.ownerDocument.body); await body.findByText("Elicitation Request"); - expect( + await expect( body.getByText("https://example.com/authorize?token=abc123"), ).toBeInTheDocument(); }, 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/ProtocolListPanel/ProtocolListPanel.stories.tsx b/clients/web/src/components/groups/ProtocolListPanel/ProtocolListPanel.stories.tsx index 2e1dfe8d..861f23c2 100644 --- a/clients/web/src/components/groups/ProtocolListPanel/ProtocolListPanel.stories.tsx +++ b/clients/web/src/components/groups/ProtocolListPanel/ProtocolListPanel.stories.tsx @@ -107,7 +107,7 @@ export const WithEntries: Story = { // bar never overlays the cards, and uses type="scroll" so it stays hidden // when idle rather than appearing on hover (#1474). play: async ({ canvasElement }) => { - expectScrollbarGutterIdleHidden(canvasElement); + await expectScrollbarGutterIdleHidden(canvasElement); }, }; diff --git a/clients/web/src/components/groups/ResourceControls/ResourceControls.stories.tsx b/clients/web/src/components/groups/ResourceControls/ResourceControls.stories.tsx index 958ec6aa..e7cd4f90 100644 --- a/clients/web/src/components/groups/ResourceControls/ResourceControls.stories.tsx +++ b/clients/web/src/components/groups/ResourceControls/ResourceControls.stories.tsx @@ -86,8 +86,8 @@ export const Default: Story = { // chevron points down. play: async ({ canvasElement }) => { const transforms = chevronTransforms(canvasElement); - expect(transforms).toHaveLength(3); - for (const t of transforms) expect(t).toBe(ROTATED_DOWN); + await expect(transforms).toHaveLength(3); + for (const t of transforms) await expect(t).toBe(ROTATED_DOWN); }, }; @@ -122,9 +122,9 @@ export const Collapsed: Story = { }, play: async ({ canvasElement }) => { const transforms = chevronTransforms(canvasElement); - expect(transforms).toHaveLength(3); + await expect(transforms).toHaveLength(3); // Identity matrix = the right-pointing arrow's natural 0° (not rotated). - for (const t of transforms) expect(t).toBe(NOT_ROTATED); + for (const t of transforms) await expect(t).toBe(NOT_ROTATED); }, }; @@ -140,10 +140,10 @@ export const EmptySectionCollapsed: Story = { }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByText("Subscriptions (0)")).toBeInTheDocument(); + await expect(canvas.getByText("Subscriptions (0)")).toBeInTheDocument(); const transforms = chevronTransforms(canvasElement); const downCount = transforms.filter((t) => t === ROTATED_DOWN).length; - expect(downCount).toBe(2); // URIs + Templates down, Subscriptions right + await expect(downCount).toBe(2); // URIs + Templates down, Subscriptions right }, }; @@ -158,11 +158,11 @@ export const SubscriptionsUnsupported: Story = { }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.queryByText(/Subscriptions/)).not.toBeInTheDocument(); + await expect(canvas.queryByText(/Subscriptions/)).not.toBeInTheDocument(); // Only the two remaining sections render, both open → both chevrons down. const transforms = chevronTransforms(canvasElement); - expect(transforms).toHaveLength(2); - for (const t of transforms) expect(t).toBe(ROTATED_DOWN); + await expect(transforms).toHaveLength(2); + for (const t of transforms) await expect(t).toBe(ROTATED_DOWN); }, }; 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/ResourceLink/ResourceLink.stories.tsx b/clients/web/src/components/groups/ResourceLink/ResourceLink.stories.tsx index 655a5e7a..ac6b6ca2 100644 --- a/clients/web/src/components/groups/ResourceLink/ResourceLink.stories.tsx +++ b/clients/web/src/components/groups/ResourceLink/ResourceLink.stories.tsx @@ -45,10 +45,12 @@ export const Static: Story = { }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByText(URI)).toBeInTheDocument(); + await expect(canvas.getByText(URI)).toBeInTheDocument(); // Copy button is present; there's no expand control in the static card. - expect(canvas.getByRole("button", { name: "Copy" })).toBeInTheDocument(); - expect( + await expect( + canvas.getByRole("button", { name: "Copy" }), + ).toBeInTheDocument(); + await expect( canvas.queryByRole("button", { name: /^Expand resource/ }), ).not.toBeInTheDocument(); }, diff --git a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.test.tsx b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.test.tsx index 596a9340..083a11c6 100644 --- a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.test.tsx +++ b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.test.tsx @@ -27,6 +27,11 @@ const noVarTemplate: ResourceTemplate = { uriTemplate: "file:///static.txt", }; +const queryTemplate: ResourceTemplate = { + name: "Events", + uriTemplate: "foobar://events{?topic}", +}; + describe("ResourceTemplatePanel", () => { it("renders the template title (or name) and description", () => { renderWithMantine( @@ -154,6 +159,200 @@ describe("ResourceTemplatePanel", () => { ).not.toBeDisabled(); }); + describe("RFC 6570 expansion (#1919)", () => { + it("renders an input for a query expression the old regex could not see", () => { + renderWithMantine( + , + ); + expect(screen.getByLabelText("topic")).toBeInTheDocument(); + }); + + it("percent-encodes a reserved character in a simple variable", async () => { + const user = userEvent.setup(); + const onReadResource = vi.fn(); + renderWithMantine( + , + ); + await user.type(screen.getByLabelText("userId"), "foo/bar"); + await user.click(screen.getByRole("button", { name: "Read Resource" })); + expect(onReadResource).toHaveBeenCalledWith( + "file:///users/foo%2Fbar/profile", + ); + }); + + it("builds an encoded query expression for {?topic}", async () => { + const user = userEvent.setup(); + const onReadResource = vi.fn(); + renderWithMantine( + , + ); + await user.type(screen.getByLabelText("topic"), "foo/bar"); + await user.click(screen.getByRole("button", { name: "Read Resource" })); + expect(onReadResource).toHaveBeenCalledWith( + "foobar://events?topic=foo%2Fbar", + ); + }); + + it("marks a query variable Optional and does not gate the read on it", async () => { + const user = userEvent.setup(); + const onReadResource = vi.fn(); + renderWithMantine( + , + ); + expect(screen.getByText("Optional")).toBeInTheDocument(); + const button = screen.getByRole("button", { name: "Read Resource" }); + expect(button).not.toBeDisabled(); + // Left blank, the whole expression drops out per RFC 6570. + await user.click(button); + expect(onReadResource).toHaveBeenCalledWith("foobar://events"); + }); + + it("does not mark a required simple variable Optional", () => { + renderWithMantine( + , + ); + expect(screen.queryByText("Optional")).not.toBeInTheDocument(); + }); + + it("previews the query expression verbatim until it is filled", async () => { + const user = userEvent.setup(); + renderWithMantine( + , + ); + expect(screen.getByText("foobar://events{?topic}")).toBeInTheDocument(); + await user.type(screen.getByLabelText("topic"), "news"); + expect( + screen.getByText("foobar://events?topic=news"), + ).toBeInTheDocument(); + }); + + // A template the server advertised can be malformed. There is no URI to + // send for one, so the read is withheld and the reason shown -- reading the + // raw template with its braces intact would draw a confusing "not found" + // from the server for a defect that is not the user's. + it.each([ + ["an out-of-grammar prefix modifier", "x://items/{id:abc}"], + ["an expression declaring no variable", "x://items/{}"], + ])("refuses to read a template with %s", (_label, uriTemplate) => { + const onReadResource = vi.fn(); + renderWithMantine( + , + ); + expect( + screen.getByRole("button", { name: "Read Resource" }), + ).toBeDisabled(); + expect(screen.getByText(/Invalid RFC 6570 varspec/)).toBeInTheDocument(); + expect(onReadResource).not.toHaveBeenCalled(); + }); + + it("names every unmet group, so a disabled button always has a reason", async () => { + // With `{a,b}{b,c}{a,c}`, filling `b` satisfies the first two groups. A + // per-field hint then looks satisfied while Read Resource stays disabled + // on the third -- so the outstanding requirement is stated form-level. + const user = userEvent.setup(); + renderWithMantine( + , + ); + expect(screen.getByText(/Still needed:/)).toHaveTextContent( + "Still needed: a or b; b or c; a or c", + ); + await user.type(screen.getByLabelText("b"), "1"); + expect(screen.getByText(/Still needed:/)).toHaveTextContent( + "Still needed: a or c", + ); + expect( + screen.getByRole("button", { name: "Read Resource" }), + ).toBeDisabled(); + await user.type(screen.getByLabelText("a"), "2"); + expect(screen.queryByText(/Still needed:/)).not.toBeInTheDocument(); + expect( + screen.getByRole("button", { name: "Read Resource" }), + ).toBeEnabled(); + }); + + it("hints every shared group a variable sits in, not just the first", () => { + renderWithMantine( + , + ); + expect( + screen.getByText("Any one of each: a, b; a, c"), + ).toBeInTheDocument(); + }); + + it("treats a name repeated in one expression as a plain requirement", () => { + renderWithMantine( + , + ); + // Not "Any one of: a, a" -- it is one field, required. + expect(screen.queryByText(/Any one of/)).not.toBeInTheDocument(); + expect(screen.getByText("Still needed: a")).toBeInTheDocument(); + }); + + it("renders an autocomplete field for a name that collides with Object.prototype", async () => { + // `toString` is a valid RFC 6570 varname and `completions` starts empty, + // so a bare `completions[varName] ?? []` handed Mantine the prototype's + // *function* as its options array (`??` catches only null/undefined) and + // the field crashed on first render. + const user = userEvent.setup(); + const onCompleteArgument = vi.fn().mockResolvedValue(["a", "b"]); + const onReadResource = vi.fn(); + renderWithMantine( + , + ); + // By role, not label: "toString" also appears in the "Still needed" line. + const input = screen.getByRole("textbox", { name: "toString" }); + expect(input).toBeInTheDocument(); + await user.type(input, "v"); + await user.click(screen.getByRole("button", { name: "Read Resource" })); + expect(onReadResource).toHaveBeenCalledWith("x://v"); + }); + + it("shows the malformed template unexpanded in the preview", () => { + renderWithMantine( + , + ); + expect(screen.getByText("x://items/{}")).toBeInTheDocument(); + }); + }); + describe("completions", () => { it("fires a completion immediately on focus before any keystroke", async () => { const user = userEvent.setup(); diff --git a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx index 6b506228..e3884583 100644 --- a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx +++ b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx @@ -14,6 +14,14 @@ import { useValueChange } from "../../../hooks/useValueChange"; import type { ResourceTemplateType as ResourceTemplate } from "@modelcontextprotocol/client"; import { AnnotationBadge } from "../../elements/AnnotationBadge/AnnotationBadge"; import { CopyButton } from "../../elements/CopyButton/CopyButton"; +import { + definedValues, + previewUriTemplate, + requiredGroups, + templateVariables, + tryExpandUriTemplate, + unmetRequiredGroups, +} from "../../../utils/uriTemplate"; export interface ResourceTemplatePanelProps { template: ResourceTemplate; @@ -39,32 +47,22 @@ export interface ResourceTemplatePanelProps { const COMPLETION_DEBOUNCE_MS = 300; -function parseVariableNames(uriTemplate: string): string[] { - const names: string[] = []; - const regex = /\{(\w+)\}/g; - let match: RegExpExecArray | null; - - while ((match = regex.exec(uriTemplate)) !== null) { - names.push(match[1]); - } - - return names; -} - -function resolveUri( - uriTemplate: string, - variables: Record, -): string { - return uriTemplate.replace(/\{(\w+)\}/g, (_, key: string) => variables[key]); -} - -function previewUri( - uriTemplate: string, - variables: Record, -): string { - return uriTemplate.replace(/\{(\w+)\}/g, (match, key: string) => - variables[key]?.length > 0 ? variables[key] : match, - ); +/** + * The completions fetched for one variable, ignoring anything inherited from + * `Object.prototype`. + * + * `toString`, `constructor` and `__proto__` are valid RFC 6570 variable names, + * and this map starts empty — so a bare `completions[varName] ?? []` returned + * the prototype's *function* for such a name (`??` only catches null and + * undefined) and handed it to Mantine as its `data` array, crashing the field + * on first render. Same hazard the expansion path fixed for values; this is + * the one place the component reads a name-keyed map it did not seed. + */ +function completionsFor( + completions: Record, + varName: string, +): string[] { + return Object.hasOwn(completions, varName) ? completions[varName] : []; } const HeaderRow = Group.withProps({ @@ -90,6 +88,26 @@ const DescriptionText = Text.withProps({ c: "dimmed", }); +// Why Read Resource is disabled when the template itself cannot be expanded. +// Without it the button is inert with nothing on screen explaining the refusal. +// +// `role="alert"` because the message can appear *while typing* -- a pasted +// value that cannot be encoded -- so a screen-reader user would otherwise meet +// a silently disabled action. An alert is an assertive live region, which is +// what announces text that arrives after first render. +// What is still missing before Read Resource can fire. Dimmed rather than red: +// an incomplete form is the expected starting state, not an error. +const RequirementText = Text.withProps({ + size: "sm", + c: "dimmed", +}); + +const ExpansionErrorText = Text.withProps({ + size: "sm", + c: "red", + role: "alert", +}); + // Left-aligned so the action sits closest to the sidebar controls / the form // fields above; annotation badges trail it. const FooterRow = Group.withProps({ @@ -108,10 +126,20 @@ export function ResourceTemplatePanel({ }: ResourceTemplatePanelProps) { const { name, title, uriTemplate, description, annotations } = template; - const variableNames = useMemo( - () => parseVariableNames(uriTemplate), + // Every variable the template declares, with the operator it appears under + // and whether omitting it would change the URI's shape (see `utils/uriTemplate`). + const declaredVariables = useMemo( + () => templateVariables(uriTemplate), [uriTemplate], ); + const variableNames = useMemo( + () => declaredVariables.map((v) => v.name), + [declaredVariables], + ); + // The names of each expression that cannot be omitted. Kept separate from + // `declaredVariables` (which is deduplicated for rendering) because each + // required expression has to be satisfied on its own — see `requiredGroups`. + const groups = useMemo(() => requiredGroups(uriTemplate), [uriTemplate]); const [variables, setVariables] = useState>(() => Object.fromEntries(variableNames.map((n) => [n, ""])), @@ -196,7 +224,8 @@ export function ResourceTemplatePanel({ // show ghost suggestions from the old keystroke while the new // request is in flight (300ms debounce + network latency). setCompletions((prev) => { - if (prev[varName] === undefined) return prev; + // Own-property, as above: an inherited member is not a stale dropdown. + if (!Object.hasOwn(prev, varName)) return prev; const next = { ...prev }; delete next[varName]; return next; @@ -232,13 +261,40 @@ export function ResourceTemplatePanel({ void runCompletion(varName, value, buildContext(varName)); } - const canSubmit = variableNames.every((n) => variables[n]?.length > 0); + // Two independent gates on the read. + // + // First, whether the values cover what expansion structurally needs. Only the + // expressions whose absence would change the URI's shape count; an unfilled + // `{?topic}` is a legitimate request for the unfiltered resource, and RFC 6570 + // drops the whole expression for it. The rule is per-expression rather than + // per-variable -- `{a,b}` with only `a` filled expands to `a`'s value -- so it + // lives in core beside the expander. + // + // Second, whether the template expands at all. A template the server + // advertised can be malformed (`{id:abc}`, `{a,}`) and a pasted value can be + // unencodable, and in neither case is there a URI to send -- so withhold the + // request and say why, rather than reading the raw template with its braces + // intact and letting the server answer with a confusing "not found". + // `definedValues` is applied here rather than inside the expander: a key + // present with `""` is a *defined* RFC 6570 value and legitimately expands to + // `?topic=`, but this form seeds every declared variable with `""`, so an + // untouched field is indistinguishable from a deliberately empty one. That is + // a fact about the form, so the form is what resolves it. + const expansion = tryExpandUriTemplate(uriTemplate, definedValues(variables)); + // Named rather than merely counted, so a disabled Read Resource always has a + // reason on screen. Per-field hints cannot carry this: a variable shared + // across several groups looks satisfied once any one of them is met. + const unmet = unmetRequiredGroups(groups, variables); + const canSubmit = expansion.error === undefined && unmet.length === 0; function handleSubmit() { - onReadResource(resolveUri(uriTemplate, variables)); + /* v8 ignore next -- unreachable: the button is disabled whenever the + expansion failed, which is the only way `uri` is undefined. */ + if (expansion.uri === undefined) return; + onReadResource(expansion.uri); } - const preview = previewUri(uriTemplate, variables); + const preview = previewUriTemplate(uriTemplate, variables); return ( @@ -251,16 +307,47 @@ export function ResourceTemplatePanel({ {description && {description}} - {variableNames.map((varName) => { + {declaredVariables.map(({ name: varName, required }) => { /* v8 ignore next -- `?? ""` fallback unreachable: `variables` is seeded with every declared variable, so the key is always present. */ const fieldValue = variables[varName] ?? ""; + // RFC 6570 omits an undefined variable under a query/path-segment + // operator entirely, so those fields are genuinely optional. In a + // required multi-name expression no single field is mandatory either + // -- any one of them satisfies it -- so say which, rather than + // marking each one required and blocking valid input. + // A name can sit in a singleton required group *and* a shared one + // (`x://{a}/{a,b}`). The singleton demands this exact field, so the + // "any one of" hint would contradict the disabled submit button -- + // suppress it and let the field read as plainly required. + const individuallyRequired = groups.some( + (names) => names.length === 1 && names[0] === varName, + ); + // EVERY shared group this variable sits in, not the first. With + // `{a,b}{b,c}{a,c}`, showing only the first left `b` looking like it + // satisfied everything while Read Resource stayed disabled on the + // unmet `{a,c}` -- a hidden requirement is worse than a wordy hint. + // The form-level "Still needed" line below names what is actually + // outstanding, which is the part a per-field hint cannot express. + const sharedGroups = individuallyRequired + ? [] + : groups.filter( + (names) => names.length > 1 && names.includes(varName), + ); + const description = !required + ? "Optional" + : sharedGroups.length > 0 + ? `Any one of${sharedGroups.length > 1 ? " each" : ""}: ${sharedGroups + .map((names) => names.join(", ")) + .join("; ")}` + : undefined; return useAutocomplete ? ( @@ -290,6 +378,14 @@ export function ResourceTemplatePanel({ ); })} + {expansion.error !== undefined && ( + {expansion.error} + )} + {expansion.error === undefined && unmet.length > 0 && ( + + Still needed: {unmet.map((names) => names.join(" or ")).join("; ")} + + )} + + ); + } + 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 ( { />, ); await user.click(screen.getByRole("button", { name: "Custom Headers" })); - const removeButtons = screen.getAllByRole("button", { name: "X" }); - await user.click(removeButtons[0]); + await user.click( + screen.getByRole("button", { + name: "Remove header, Authorization, row 1", + }), + ); expect(onSettingsChange).toHaveBeenCalledWith({ ...initialSettings, headers: [], @@ -342,10 +345,13 @@ describe("ServerSettingsModal", () => { />, ); await user.click(screen.getByRole("button", { name: "Request Metadata" })); - const removeButtons = screen.getAllByRole("button", { name: "X" }); - // After expanding metadata, both header and metadata X buttons exist; - // the metadata X is the last one. - await user.click(removeButtons[removeButtons.length - 1]); + // Both the header and metadata rows have a remove button; each is named + // for the row it belongs to, so no positional guess is needed. + await user.click( + screen.getByRole("button", { + name: "Remove metadata entry, userId, row 1", + }), + ); expect(onSettingsChange).toHaveBeenCalledWith({ ...initialSettings, metadata: [], @@ -440,10 +446,81 @@ describe("ServerSettingsModal", () => { oauthClientId: "", oauthClientSecret: "", oauthScopes: "", + oauthAuthorizationParams: [], enterpriseManaged: true, }); }); + // #1906 — a typed endpoint override lands on the settings; clearing it + // persists `undefined` rather than an empty string, so "cleared" and "never + // set" are the same state on disk. + it("persists a typed endpoint override, and clears it back to undefined", async () => { + const user = userEvent.setup(); + const onSettingsChange = vi.fn(); + const { rerender } = renderWithMantine( + , + ); + await user.click(screen.getByRole("button", { name: "OAuth Settings" })); + await user.type( + screen.getByRole("textbox", { name: /Token URL override/i }), + "h", + ); + expect(onSettingsChange).toHaveBeenCalledWith( + expect.objectContaining({ oauthTokenUrl: "h" }), + ); + + onSettingsChange.mockClear(); + rerender( + , + ); + const clearButtons = screen.getAllByRole("button", { name: /clear/i }); + await user.click(clearButtons[clearButtons.length - 1]); + expect(onSettingsChange).toHaveBeenCalledWith( + expect.objectContaining({ oauthTokenUrl: undefined }), + ); + }); + + // #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(); @@ -739,8 +816,11 @@ describe("ServerSettingsModal", () => { await user.click( screen.getByRole("button", { name: "Environment Variables" }), ); - const removeButtons = screen.getAllByRole("button", { name: "X" }); - await user.click(removeButtons[removeButtons.length - 1]); + await user.click( + screen.getByRole("button", { + name: "Remove environment variable, A, row 1", + }), + ); expect(onSettingsChange).toHaveBeenCalledWith({ ...emptySettings, env: [], diff --git a/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.tsx b/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.tsx index e391c240..35c1fb12 100644 --- a/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.tsx +++ b/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.tsx @@ -179,6 +179,15 @@ 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, + // #1906: an emptied field persists as undefined rather than `""` — the + // read side treats a blank override as "not configured", and keeping the + // two in step means clearing the input really clears the setting. + oauthAuthorizationUrl: oauth.authorizationUrl || undefined, + oauthTokenUrl: oauth.tokenUrl || undefined, 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/StructuredOutputPanel/StructuredOutputPanel.stories.tsx b/clients/web/src/components/groups/StructuredOutputPanel/StructuredOutputPanel.stories.tsx index c66760f0..78f1ea8d 100644 --- a/clients/web/src/components/groups/StructuredOutputPanel/StructuredOutputPanel.stories.tsx +++ b/clients/web/src/components/groups/StructuredOutputPanel/StructuredOutputPanel.stories.tsx @@ -41,7 +41,7 @@ export const Nested: Story = { }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect( + await expect( canvas.getByRole("heading", { name: "Structured Output" }), ).toBeInTheDocument(); // The section starts expanded, so the payload is visible without a click. @@ -78,9 +78,9 @@ export const Large: Story = { } // Capped: the visible height stays at the section's `mah`, well under the // payload's natural height… - expect(viewport.clientHeight).toBeLessThanOrEqual(400); + await expect(viewport.clientHeight).toBeLessThanOrEqual(400); // …and the overflow is reachable by scrolling rather than clipped away. - expect(viewport.scrollHeight).toBeGreaterThan(viewport.clientHeight); + await expect(viewport.scrollHeight).toBeGreaterThan(viewport.clientHeight); }, }; @@ -95,7 +95,7 @@ export const Collapsed: Story = { const toggle = canvas.getByRole("button", { name: "Expand structured output", }); - expect(toggle).toHaveAttribute("aria-expanded", "false"); + await expect(toggle).toHaveAttribute("aria-expanded", "false"); await userEvent.click(toggle); await waitFor(() => expect( diff --git a/clients/web/src/components/groups/ToolControls/ToolControls.stories.tsx b/clients/web/src/components/groups/ToolControls/ToolControls.stories.tsx index 23709132..9989e9ff 100644 --- a/clients/web/src/components/groups/ToolControls/ToolControls.stories.tsx +++ b/clients/web/src/components/groups/ToolControls/ToolControls.stories.tsx @@ -45,7 +45,8 @@ export const Default: Story = { export const WithSelection: Story = { args: { tools: longToolList, - selectedName: "query_database", + // Row identity is `index:name`, so the selection names the position too. + selectedKey: "1:query_database", }, }; diff --git a/clients/web/src/components/groups/ToolControls/ToolControls.test.tsx b/clients/web/src/components/groups/ToolControls/ToolControls.test.tsx index b6e379e8..a9a17698 100644 --- a/clients/web/src/components/groups/ToolControls/ToolControls.test.tsx +++ b/clients/web/src/components/groups/ToolControls/ToolControls.test.tsx @@ -83,7 +83,7 @@ describe("ToolControls", () => { , ); await user.click(screen.getByText("git_status")); - expect(onSelectTool).toHaveBeenCalledWith("git_status"); + expect(onSelectTool).toHaveBeenCalledWith("2:git_status"); }); it("does not invoke onSelectTool when the already-selected tool is clicked", async () => { @@ -92,7 +92,7 @@ describe("ToolControls", () => { renderWithMantine( , ); @@ -100,6 +100,40 @@ describe("ToolControls", () => { expect(onSelectTool).not.toHaveBeenCalled(); }); + it("selects each copy of a duplicated tool name independently", async () => { + // A `tools/list` may repeat a name. Selection is keyed by row identity, so + // selecting the first copy must leave the second clickable and unhighlighted + // (#2001 — with a name-keyed selection both highlighted and neither of the + // later copies could be opened). + const user = userEvent.setup(); + const onSelectTool = vi.fn(); + const duplicated: Tool[] = [ + { name: "get_weather", inputSchema: { type: "object" } }, + { name: "echo", inputSchema: { type: "object" } }, + { name: "get_weather", inputSchema: { type: "object" } }, + ]; + renderWithMantine( + , + ); + const rows = screen.getAllByText("get_weather"); + expect(rows).toHaveLength(2); + // Only the selected row carries the highlight background. + const highlighted = rows + .map((row) => row.closest("button")) + .filter((button) => button?.style.background !== ""); + expect(highlighted).toHaveLength(1); + expect(highlighted[0]).toBe(rows[0]?.closest("button")); + + // The second copy is still clickable, and reports its own row key. + await user.click(rows[1] as HTMLElement); + expect(onSelectTool).toHaveBeenCalledWith("2:get_weather"); + }); + it("does not show the list-changed indicator when listChanged is false", () => { renderWithMantine(); expect(screen.queryByText("List updated")).not.toBeInTheDocument(); diff --git a/clients/web/src/components/groups/ToolControls/ToolControls.tsx b/clients/web/src/components/groups/ToolControls/ToolControls.tsx index 4ad70d85..92adc2d5 100644 --- a/clients/web/src/components/groups/ToolControls/ToolControls.tsx +++ b/clients/web/src/components/groups/ToolControls/ToolControls.tsx @@ -15,19 +15,27 @@ 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, } from "../../elements/ListPaginationControls/ListPaginationControls"; import { ToolListItem } from "../ToolListItem/ToolListItem"; import { useScrollMemory } from "../../../hooks/useScrollMemory"; +import { toolRowKey } from "../../../utils/toolUtils"; export interface ToolControlsProps { tools: Tool[]; /** Tools the SDK excluded from `tools/list` for invalid `x-mcp-header` * annotations (SEP-2243), shown below the list with the reason (#1632). */ excludedTools?: ExcludedTool[]; - selectedName?: string; + /** + * The selected row's {@link toolRowKey}, not its name — a `tools/list` may + * repeat a name, and a name-keyed selection would highlight every copy at + * once and make the others unclickable (#2001). + */ + selectedKey?: string; // Search text is controlled by the parent (App, via ToolsScreen) so it // persists across tab navigation within a live session — see #1417. searchText?: string; @@ -37,11 +45,18 @@ 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; onSearchChange: (value: string) => void; - onSelectTool: (name: string) => void; + /** Emits the clicked row's {@link toolRowKey} (see `selectedKey`). */ + onSelectTool: (key: string) => void; } // One excluded tool: a warning icon, the tool name (struck through, since it is @@ -108,12 +123,12 @@ const ExcludedTooltip = Tooltip.withProps({ position: "right", }); -// A server may return the same tool name more than once, so the name alone is -// not a unique React key — colliding keys let a filtered-out row survive -// reconciliation instead of unmounting (#1957). The tool's position in the -// unfiltered list disambiguates duplicates and stays stable while the search -// narrows, since it is captured before filtering. -const rowKey = (name: string, sourceIndex: number) => `${sourceIndex}:${name}`; +// Excluded rows render into the same `Stack` as the list rows above them, so +// their keys share one namespace — and an excluded tool's index is counted +// within its own list, which would collide with a listed tool of the same name +// at the same index. The prefix keeps the two spaces apart. +const excludedRowKey = (name: string, sourceIndex: number) => + `excluded:${toolRowKey(name, sourceIndex)}`; /** Matches a tool against the (already lower-cased) search query by name or title. */ const matchesQuery = (tool: Tool, query: string) => @@ -123,10 +138,11 @@ const matchesQuery = (tool: Tool, query: string) => export function ToolControls({ tools, excludedTools = [], - selectedName, + selectedKey, searchText = "", listChanged, onRefreshList, + malformedListItems = [], loadError, pagination, onSearchChange, @@ -137,14 +153,17 @@ export function ToolControls({ // Stamp each row's source position before filtering, so the key survives the // list narrowing (#1957). const filteredTools = tools - .map((tool, sourceIndex) => ({ tool, key: rowKey(tool.name, sourceIndex) })) + .map((tool, sourceIndex) => ({ + tool, + key: toolRowKey(tool.name, sourceIndex), + })) .filter(({ tool }) => !searchText || matchesQuery(tool, query)); // Excluded tools are searchable too, matching name AND title like the main // list above, so a filtered view stays consistent. const filteredExcluded = excludedTools .map((excluded, sourceIndex) => ({ ...excluded, - key: rowKey(excluded.tool.name, sourceIndex), + key: excludedRowKey(excluded.tool.name, sourceIndex), })) .filter(({ tool }) => !searchText || matchesQuery(tool, query)); @@ -163,15 +182,20 @@ export function ToolControls({ /> + {filteredTools.map(({ tool, key }) => ( { - if (tool.name !== selectedName) onSelectTool(tool.name); + if (key !== selectedKey) onSelectTool(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({