diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index e6a22c98..66c1ef1e 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -78,9 +78,9 @@ Both exist and do different jobs. Theme files (`src/theme/.ts`) custo - **The MCP SDK packages (`@modelcontextprotocol/client`, `core`, `server`, `server-legacy`, `ext-apps`) belong in the root `package.json` only.** Adding one to a `clients/*/package.json` is a review finding: Node resolution walks up, so the root install already serves every client, and a per-client entry installs a second copy that drifts (it produced two versions of `ext-apps`, and of the transitive v1 SDK, at once — #1970) and reintroduces the duplicate-copy failure `vitest.shared.mts` has a `dedupe` workaround for. - **The v1 `@modelcontextprotocol/sdk` is not a dependency of this repo** and must not become one. It is a peer of `ext-apps`, present in lock files only. - Dependencies reached only through **root-owned code with no manifest** (`test-servers/src`, `core/`) are declared at the root and aliased to the **repo root** in `vitest.shared.mts` — as `express` and `yaml` are — not to `/node_modules` like the other pins there. -- **A dependency that renders React components must be bundled into the client that uses it, and is then not a root dependency.** An externalized package resolves its own `react` from wherever npm placed it in the *consumer's* tree, beside a React satisfying *its* peer range — looser than ours, which is all it takes to split React. `ink-form`/`ink-scroll-view` declare `">=18"`, so a consumer's React 18 satisfies them, the TUI ends up with two React instances, and it crashes on the first hook (#1952). Both are inlined via `noExternal` in `clients/tui/tsup.config.ts`. **`ink` is the one exemption, justified by cost (~1.4MB) — never by a peer range**: flag any claim that `">=19"` keeps npm from misplacing it, which is false and was in this repo once. What keeps it safe is the **root `react` range staying open to the whole major (`^19.0.0`)** so npm can dedupe with a consumer's pinned React 19; treat narrowing that range as reopening the bug. `clients/tui/__tests__/tsupConfig.test.ts` enforces the split, the root-declaration of exempt packages, and that range. +- **A dependency that renders React components must be bundled into the client that uses it, and is then not a root dependency.** An externalized package resolves its own `react` from wherever npm placed it in the _consumer's_ tree, beside a React satisfying _its_ peer range — looser than ours, which is all it takes to split React. `ink-form`/`ink-scroll-view` declare `">=18"`, so a consumer's React 18 satisfies them, the TUI ends up with two React instances, and it crashes on the first hook (#1952). Both are inlined via `noExternal` in `clients/tui/tsup.config.ts`. **`ink` is the one exemption, justified by cost (~1.4MB) — never by a peer range**: flag any claim that `">=19"` keeps npm from misplacing it, which is false and was in this repo once. What keeps it safe is the **root `react` range staying open to the whole major (`^19.0.0`)** so npm can dedupe with a consumer's pinned React 19; treat narrowing that range as reopening the bug. `clients/tui/__tests__/tsupConfig.test.ts` enforces the split, the root-declaration of exempt packages, and that range. - **Which section is a separate question from which manifest.** A package `core/` imports at runtime must be in root **`dependencies`**: client builds externalize npm packages, so a published install resolves them from the root manifest and devDependencies are absent there. Only test/build-only packages (`express`) belong in `devDependencies`. Flag a runtime `core/` import added to `devDependencies` — it passes every local check and breaks the published package. -- **A root-declared package that `core/` imports at runtime must also be named in each client's bundler `external` list** — `clients/{cli,tui}/tsup.config.ts` and `clients/web/tsup.runner.config.ts`, all three. Bundlers externalize what the *client's* manifest declares, and these packages are root-only by rule, so omitting them means they get bundled. For a CJS package inlined into an ESM bundle that is fatal: esbuild's `Dynamic require of "path" is not supported` shim throws at import time and the binary dies before parsing a flag (#2082, `proper-lockfile`). Flag a new root runtime dependency that is not added to all three. +- **A root-declared package that `core/` imports at runtime must also be named in each client's bundler `external` list** — `clients/{cli,tui}/tsup.config.ts` and `clients/web/tsup.runner.config.ts`, all three. Bundlers externalize what the _client's_ manifest declares, and these packages are root-only by rule, so omitting them means they get bundled. For a CJS package inlined into an ESM bundle that is fatal: esbuild's `Dynamic require of "path" is not supported` shim throws at import time and the binary dies before parsing a flag (#2082, `proper-lockfile`). Flag a new root runtime dependency that is not added to all three. This has broken twice — `undici` slipped past #2082 because it _was_ declared in `clients/cli/package.json`, so the CLI looked fine while the web and TUI bundles inlined 1.05MB of it, unloadably (#2067). `npm run verify:bundle-externals` now guards it from the **built output**; flag any change that would weaken or skip that guard. ## Tests and the coverage gate @@ -95,20 +95,20 @@ Both exist and do different jobs. Theme files (`src/theme/.ts`) custo ### Rendering components in tests -- **Always render through `renderWithMantine`** from `src/test/renderWithMantine.tsx`. A hand-rolled bare `MantineProvider` skips the project theme and the helper's options, and drifts from every other test. (It does *not* reintroduce the timer-leak class — an older version of this rule said so; the leaked-timer net in `setup.ts` is global and covers every unit test regardless of how it renders.) +- **Always render through `renderWithMantine`** from `src/test/renderWithMantine.tsx`. A hand-rolled bare `MantineProvider` skips the project theme and the helper's options, and drifts from every other test. (It does _not_ reintroduce the timer-leak class — an older version of this rule said so; the leaked-timer net in `setup.ts` is global and covers every unit test regardless of how it renders.) - For a forced color scheme, pass the option — `renderWithMantine(ui, { colorScheme: "dark" })` — rather than a hand-rolled `defaultColorScheme` provider. - Only when asserting _mid-flight_ transition state, use `renderWithMantineTransitions`, passing `settleMs` derived from the component's real animation duration. Do **not** combine it with `vi.useFakeTimers()`, and use the `unmount()` it returns if the test unmounts the tree itself. ## 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. +- `npm run format` before committing; **`npm run ci` before pushing** (`validate` → `coverage` → `verify:build-gate` → `verify:bundle-externals` → `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 — 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*. +- **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`; `pack:verify`'s `--web` child sets its own `MCP_CATALOG_PATH` for the same reason (#2003 — its App deep link persists a server row). Anything that boots the web backend and then *navigates* it needs that isolation, not just the scripts named `smoke:*`. 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_. - **Lint has no warning tier.** Every `lint` script runs `--max-warnings 0`, so a warning fails `validate` exactly as an error does (#2085) — a `warn`-level `react-hooks/exhaustive-deps` finding otherwise let a stale-closure bug pass the pre-push gate and reach review. Flag a PR that silences a finding to make the gate pass (widening a `globalIgnores`, dropping a rule, or an inline disable with no justification comment); the fix is the defect, not the message. A rule meant to be enforced should be set to `error` rather than left at `warn` and carried by the flag. - **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. +- **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. - Update the relevant `README.md` / `AGENTS.md` when a change adds, removes, renames, or repurposes a file or folder, changes the structure or tech stack, or introduces a command, dependency, or architectural pattern. ## What to prioritize in review diff --git a/.github/workflows/claude.yml b/.github/workflows/claude.yml deleted file mode 100644 index b35b91c3..00000000 --- a/.github/workflows/claude.yml +++ /dev/null @@ -1,132 +0,0 @@ -name: Claude Code - -on: - issue_comment: - types: [created] - pull_request_review_comment: - types: [created] - issues: - types: [opened, assigned] - pull_request_review: - types: [submitted] - -jobs: - claude: - if: | - ( - (github.event_name == 'issue_comment' && - contains(github.event.comment.body, '@claude') && - contains(fromJSON('["OWNER", "MEMBER", "COLLABORATOR"]'), github.event.comment.author_association)) || - (github.event_name == 'pull_request_review_comment' && - contains(github.event.comment.body, '@claude') && - contains(fromJSON('["OWNER", "MEMBER", "COLLABORATOR"]'), github.event.comment.author_association)) || - (github.event_name == 'pull_request_review' && - contains(github.event.review.body, '@claude') && - contains(fromJSON('["OWNER", "MEMBER", "COLLABORATOR"]'), github.event.review.author_association)) || - (github.event_name == 'issues' && - (contains(github.event.issue.body, '@claude') || contains(github.event.issue.title, '@claude')) && - contains(fromJSON('["OWNER", "MEMBER", "COLLABORATOR"]'), github.event.issue.author_association)) - ) - runs-on: ubuntu-latest - permissions: - contents: read - pull-requests: read - issues: read - id-token: write - actions: read - steps: - # Actions are pinned to full commit SHAs rather than mutable major tags: - # this job holds ANTHROPIC_API_KEY and grants the agent Bash, so a - # force-moved tag would be an unreviewed code change inside a - # secret-holding job. The trailing `# vX.Y.Z` comment is the form - # Dependabot reads, so pinning costs us no upgrade automation. (#1882) - - # This lookup exists only to answer "is the head in a fork?". The action - # below checks out the PR branch itself, so no `sha` is needed here. - - name: Get PR details - if: | - (github.event_name == 'issue_comment' && github.event.issue.pull_request) || - github.event_name == 'pull_request_review_comment' || - github.event_name == 'pull_request_review' - id: pr - uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 - with: - script: | - let prNumber; - if (context.eventName === 'issue_comment') { - prNumber = context.issue.number; - } else { - prNumber = context.payload.pull_request.number; - } - - const pr = await github.rest.pulls.get({ - owner: context.repo.owner, - repo: context.repo.repo, - pull_number: prNumber - }); - - // A fork PR's head lives in a different repository — or in none at - // all, if the fork was deleted after the PR was opened (`head.repo` - // is then null). Neither is ours to run against, so both count as a - // fork here and the steps below decline rather than guess. - const headRepo = pr.data.head.repo?.full_name ?? null; - const isFork = headRepo !== `${context.repo.owner}/${context.repo.repo}`; - - core.setOutput('is_fork', String(isFork)); - - # A fork PR's head is untrusted code, and running a tool-enabled agent - # holding ANTHROPIC_API_KEY against it is what this declines. The gate is - # load-bearing even though we no longer check the head out ourselves: - # claude-code-action fetches a cross-repository head via - # `refs/pull/N/head` on its own (see `isCrossRepository` in its - # src/github/operations/branch.ts), so skipping the action is the only - # thing that keeps that code out of the workspace. Reviewing the base tree - # instead would only trade the problem for a confident review of the wrong - # tree, so decline visibly and leave the reason in the run. - - name: Decline fork PR - if: steps.pr.outcome == 'success' && steps.pr.outputs.is_fork == 'true' - run: | - { - echo "### Claude Code declined this pull request" - echo - echo "The head branch lives in a fork, so Claude is not run against it." - echo "Push the branch to this repository and re-trigger if a review is needed." - } >> "$GITHUB_STEP_SUMMARY" - - # Base ref only — no `ref:` input, so nothing a PR controls selects what - # is checked out. On a PR, claude-code-action then fetches and checks out - # the PR branch itself, which is why a second PR-head checkout here would - # be redundant. It was also the one thing keeping CodeQL's - # `actions/untrusted-checkout` alert open: the rule is syntactic and - # cannot see the `is_fork` gate, the job-level `author_association` check, - # or the repo's `collaborators_only` fork-PR policy. (#2069) - - name: Checkout repository - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - with: - fetch-depth: 0 - - - name: Run Claude Code - # `skipped` means the trigger was an issue or a non-PR comment. A - # same-repo PR gives `is_fork == 'false'`. A fork PR reaches here with - # `is_fork == 'true'` and both disjuncts false, so it is skipped — and - # so the action never fetches its head. A *failed* lookup is excluded - # too: this condition carries no status-check function, so GitHub - # applies an implicit `success()` and skips the step after a failed - # prior step regardless of what the comparisons say. - if: steps.pr.outcome == 'skipped' || steps.pr.outputs.is_fork == 'false' - id: claude - uses: anthropics/claude-code-action@5ef2e550a465a721f4f45e4a7d3c340c873e1dcc # v1.0.190 - with: - anthropic_api_key: ${{ secrets.ANTHROPIC_API_KEY }} - - # Allow Claude to read CI results on PRs - additional_permissions: | - actions: read - - # Trigger when assigned to an issue - assignee_trigger: "claude" - - claude_args: | - --mcp-config .mcp.json - --allowedTools "Bash,mcp__mcp-docs" - --append-system-prompt "If posting a comment to GitHub, give a concise summary of the comment at the top and put all the details in a
block. When working on MCP-related code or reviewing MCP-related changes, use the mcp-docs MCP server to look up the latest protocol documentation. For schema details, reference https://github.com/modelcontextprotocol/modelcontextprotocol/tree/main/schema which contains versioned schemas in JSON (schema.json) and TypeScript (schema.ts) formats." diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index f7ddb49a..a7191880 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -73,6 +73,17 @@ jobs: # gate. It restores the mutated entry afterward (see the script). run: npm run verify:build-gate + - name: Verify no externalized dependency was inlined (#2067) + # The mirror of the "must be bundled" rule. `undici` was declared only in + # the root and `clients/cli` manifests, so tsup — which auto-externalizes + # what the NEAREST manifest declares — inlined 1.05MB of it into the web + # and TUI bundles. Inlined CommonJS in an ESM bundle throws + # `Dynamic require of "assert" is not supported` on first use, and the + # rewritten relative specifier meant no user-side install could fix it. + # This reads the built output rather than the config, because those two + # disagreed for four releases. + run: npm run verify:bundle-externals + # Playwright chromium is installed BEFORE the smokes because # `smoke:web:browser` (the headless-browser boot smoke, #1615) drives the # prod web bundle in chromium — restoring/installing it here lets that diff --git a/AGENTS.md b/AGENTS.md index f2416823..da5ecbfa 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -15,6 +15,34 @@ v2/main/ │ │ │ # start-vite-dev-server.ts (in-process Vite starter for the launcher), │ │ │ # web-server-config.ts (env parsing + initial-config payload + banner), │ │ │ # sandbox-controller.ts (MCP Apps sandbox HTTP server), +│ │ # app-origin-controller.ts (the DEDICATED APP ORIGIN for +│ │ # `_meta.ui.domain` — #2056. The default render hands an +│ │ # app's HTML to the proxy as `srcdoc` in a frame with no +│ │ # `allow-same-origin`, so the document has an OPAQUE origin +│ │ # and every request it makes sends `Origin: null`; no CORS / +│ │ # OAuth-callback / API-key allowlist can admit that, which is +│ │ # what the spec field exists to fix. `domain` is explicitly +│ │ # HOST-DEPENDENT and the Inspector owns no domain +│ │ # infrastructure, so it treats any non-empty value as a +│ │ # REQUEST, not an address, and answers with a real loopback +│ │ # origin on its own port (MCP_APP_ORIGIN_PORT, default 6278). +│ │ # ONE SHARED origin, path-keyed per document — a real, +│ │ # allowlistable origin without a port or DNS name per app, +│ │ # and therefore NOT a per-app isolation boundary (two such +│ │ # apps share its localStorage/cookies). The inner frame is +│ │ # granted `allow-same-origin` on THIS path only, which is +│ │ # what makes the origin real; that is not a #1565 regression +│ │ # because the listener is on its own port, so the app stays +│ │ # cross-origin to both the proxy and the Inspector — the +│ │ # proxy refuses the grant if the URL's origin equals its own, +│ │ # and it is never reachable from the server-supplied +│ │ # `sandbox` string, which is still stripped. The browser +│ │ # hands the wrapped bytes back through the authenticated +│ │ # `POST /api/app-document` (core/mcp/remote/node/server.ts) +│ │ # because only it holds the MCP connection. EVERY failure — +│ │ # no listener, port never bound, older backend, network +│ │ # error — falls back to the srcdoc render with a warning +│ │ # rather than blanking the app), │ │ │ # 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: defaults to @@ -181,7 +209,11 @@ v2/main/ │ │ │ # server.json parser, strategy registry + well-known │ │ │ # paths, strategy-agnostic merge. Pure/isomorphic; │ │ │ # used by the web file-upload path + /api/import-source. -│ │ ├── node/ # Node stdio transport factory +│ │ ├── node/ # Node stdio transport factory, plus proxyFetch.ts +│ │ │ # (the shared HTTPS_PROXY/HTTP_PROXY/NO_PROXY +│ │ │ # fetch every Node client installs as its +│ │ │ # `environment.fetch`, and the web backend as the +│ │ │ # createTransportNode default — #2067) │ │ ├── remote/ # Browser HTTP/SSE transport + remote logger/fetch │ │ │ └── node/ # Hono-based remote server backend (used by remote/ above) │ │ └── state/ # InspectorClient state stores consumed by core/react/ @@ -263,6 +295,13 @@ The same **placement** rule covers anything reached only through **root-owned co **A root-declared package that `core/` imports at runtime must also be named in each client's bundler `external` list.** tsup and Vite externalize what the *client's* `package.json` declares, and a root-only dependency is in none of them — so it gets **bundled**, silently, and the placement rule above is what guarantees every such package is root-only. For a CJS package inlined into an ESM bundle that is fatal rather than merely wasteful: esbuild leaves a `Dynamic require of "path" is not supported` shim that throws at *import* time, so the binary dies before it parses a flag. `proper-lockfile` hit exactly that in #2082; `@napi-rs/keyring` is listed in all three for the same reason. The three lists are `clients/cli/tsup.config.ts`, `clients/tui/tsup.config.ts`, and `clients/web/tsup.runner.config.ts` — add a new package to **all** of them, since which client reaches it is a function of what `core/` imports, not of what the client's own code names. +**That rule has now been broken twice, so it is enforced by a guard rather than by review.** `undici` predates #2082 and was missed by it: it *was* declared in `clients/cli/package.json`, so tsup externalized it for the CLI and the CLI looked fine, while the web and TUI bundles silently inlined 1.05MB of it (#2067). The failure mode is slightly worse than the `proper-lockfile` one, because the package is `import()`ed lazily rather than at startup: the binary boots normally and only the proxied code path dies — with esbuild's rewritten specifier (`import("./undici-HXPKCIY3.js")`) meaning **no user-side install can ever satisfy it**, so the published clients told every proxy user to install a package that was already present and could not have helped. + +**`npm run verify:bundle-externals`** (`scripts/verify-bundle-externals.mjs`, in `npm run ci` and the GitHub workflow) is that guard. It reads the **built output**, not the config — the two disagreed for four releases — and checks the union of two independently-derived candidate lists: each client's own `external` array, **and the root manifest's `dependencies`**. The second is what makes it cover #2067 rather than only its regression: `undici` was *missing* from the web and TUI `external` lists — that omission was the bug — so a guard driven by those lists alone would have inspected the inlined 1.05MB bundle and reported success. Removing a package from an `external` list must not remove it from scrutiny. Deriving both ways also means a newly added root dependency, or a newly externalized package, is covered without editing the guard. Detection keys off esbuild's `// ` module banners, which covers both shapes an inlining can take: a separate `-HASH.js` chunk (what a dynamically `import()`ed CommonJS package produces, i.e. #2067) and a statically-imported package folded straight into `index.js`, which emits no chunk and a chunk-name check would miss. Reading banners means the guard needs unminified output, which it asserts rather than assumes — a bundle with no banners fails loudly instead of reporting clean, so turning on `minify` surfaces here. It **fails closed** on the other way it could lose sight of a bundle, too: parsing the `external` array keys off literal config text, so if a tsup refactor leaves it parsing nothing, that is an error rather than a vacuous pass. Two more things worth knowing: + +- ⚠️ **Nothing in the ordinary test tiers catches this class.** Unit and integration tests run against **source**, where the real package is on the resolution path; the smokes run the built tree but never take the code path (for `undici`, no proxy env var is set), so the lazy `import()` never executes. The defect exists only in bundled output, only on a path the smokes don't walk. +- ⚠️ **Probing it by hand with `node -e` falsely succeeds.** `node -e 'import("./undici-XXXX.js")'` resolves fine, because `node -e` exposes a global `require` that satisfies esbuild's `typeof require !== "undefined"` guard. Reproduce from a real `.mjs` file, or you will conclude the chunk is loadable when it is not. + **A dependency that renders React components must be bundled into the client that uses it, and is then not a root dependency.** An externalized package resolves its own `react` from wherever npm placed **it** in the consumer's tree, and npm places a package beside a React satisfying *that package's* peer range — looser than ours in every case here, which is all it takes to split React. `ink-form` and `ink-scroll-view` declare `">=18"`, satisfied by a consumer's React 18 while our React 19 nests underneath: the bundle renders through one React, those packages call hooks on another, and the TUI crashes on the first hook (#1952). Both are inlined by `clients/tui/tsup.config.ts` (`noExternal`) and declared only in `clients/tui/package.json`, where the build resolves them — declaring an inlined package at the root would just make consumers install a second, unused copy. **`ink` is the single exemption, and it is justified by cost, not by safety.** Bundling it works but adds ~1.4MB (`react-reconciler` + `yoga-layout`, plus a `createRequire` banner, since inlined CJS calls `require` at runtime and esbuild's ESM interop rejects that without a real `require` in scope). **Never justify an exemption by a peer range** — `ink` briefly carried "its `">=19"` peer keeps npm honest", which is false: a consumer pinning React 19.0 satisfies `">=19"` while a narrower range of ours nests underneath. What actually makes the exemption safe is a *different* lever: the **root `react` range stays open to the whole major (`^19.0.0`)**, so npm can dedupe our React with whatever React 19 a consumer pins and an external `ink` lands on the same copy the bundle uses. Narrowing it (e.g. back to `^19.2.4`) silently reopens the crash for the renderer itself, which breaks TUI *startup*, not just its forms. `clients/tui/__tests__/tsupConfig.test.ts` enforces all of it: React-rendering deps inlined, each exempt package both external and root-declared, and the root range pinned to `ink`'s peer floor. @@ -800,21 +839,22 @@ The ⚠️ option-deletion hazard, the snapshot rule, and the recovery recipe ab ### Mandatory pre-push gate - 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). +- **`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) → `verify:bundle-externals` (the #2067 must-not-bundle 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 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 **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` / `smoke:web:elicit`, 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). + - **`npm run verify:bundle-externals`** runs after `verify:build-gate` in `npm run ci`. It asserts that no package a client declares `external` was inlined into that client's bundle anyway — the must-not-bundle invariant above (#2067). It reads `clients/*/build`, so it needs a build to have run (`validate` provides one). +- **`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` / `smoke:web:elicit`, 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 `verify:bundle-externals` (the #2067 must-not-bundle gate), 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. - `smoke:web` (`scripts/smoke-web.mjs`) starts `mcp-inspector --web` (prod, no `--dev`) against the built `clients/web/dist` and asserts `GET /` serves the SPA (HTTP 200) with the injected `__INSPECTOR_API_TOKEN__`. Prod `--web` serves from `clients/web/dist`, which ships in the published package but is absent in a fresh checkout — the runner builds it on demand (`build:client` = `vite build`) on first launch, or exits with an actionable error if that build can't run (see `clients/web/server/ensure-web-build.ts` and the launcher README). `--dev` runs Vite directly and never needs `dist`. It shares the spawn/readiness/teardown helper (`scripts/lib/prod-web-server.mjs`) with **`smoke:web:browser`, `smoke:web:app` and `smoke:web:elicit`**, so the four can't drift. **Every web smoke runs against a throwaway catalog (#1977).** The helper mints a temp dir per run and passes it as `MCP_CATALOG_PATH`; without it the web backend falls back to the developer's real `~/.mcp-inspector/mcp.json`, which made these smokes both destructive and non-deterministic — `smoke:web:app`'s deep link persists a `deep-link` server row, so a *second* run found it already on disk, raced hydration, and drew a spurious (swallowed, non-fatal) 409 that was really just residue from the previous run. CI never saw it: a fresh `HOME` per run made every CI run look like a first run. This matches `smoke:cli` / `smoke:tui`, which have always driven a temp `--catalog`. Only the **catalog** is redirected — other per-user state under `~/.mcp-inspector` (OAuth tokens, `storage/`) stays shared, because isolating it means redirecting `HOME` wholesale, which would also move the npx and Playwright caches these smokes depend on. Teardown uses **both** halves of `scripts/lib/child-cleanup.mjs` (`stopChild` to await the child's exit, then `removeSafe` to delete the dir) — a bare `kill()` only *delivers* the signal, so removing synchronously re-enters the #1801 ENOTEMPTY race. That makes `stop()` **async**, so every caller must `await` it (and a caller's own `fail()`/`shutdown()` becomes async in turn, or execution runs past the intended exit). The isolation contract is unit-tested in `scripts/lib/prod-web-server.test.mjs` via `test:scripts`, since the smokes exit immediately after teardown and so cannot detect a regression that silently reshared the catalog or stopped cleaning up: `createTempCatalog` and `buildWebServerEnv` cover *which* catalog the server gets, and `teardownWebServer` — extracted from `stop()` for exactly this reason — is driven against a stand-in child process so the teardown asserts on the real directory rather than a spy. - `smoke:web:browser` (`scripts/smoke-web-browser.mjs`, #1615) goes a step further than `smoke:web`: it boots the same prod `--web` server and then actually **runs** the bundle in headless Chromium (Playwright — already a `clients/web` devDependency for the Storybook tests), asserting the app renders its first meaningful frame (the "Add Servers" control) with **no uncaught error**. `smoke:web` only checks the served HTML, so a Node built-in reaching the browser bundle slipped through it; this smoke catches that regression as a _class_ (e.g. #1612). The mechanism is the uncaught error, not a magic string: under Vite the excluded module becomes an empty stub and the first _call_ into it (e.g. `fs.readFileSync(...)` during a transitive module's init) throws a `TypeError` that aborts app mount. A _synchronous_ such throw fires `pageerror`; its _async_ twin (the same `TypeError` via `await`/`.then()`, or a failed dynamic import) is logged on the console channel as `Uncaught (in promise) …` / `Failed to fetch dynamically imported module` — the smoke hard-fails on both. The literal `Module "…" has been externalized` text is, **in a prod build**, a build-time warning (`vite build` / `npm run build`), not a runtime message, so the browser never sees it (under `npm run dev` Vite's stub is instead a `Proxy` that `console.warn`s that string at runtime); and an externalized import that is never _called_ ships a harmless `{}` and is invisible here by design. Every _other_ console error is printed as a diagnostic, not a failure (so a benign font-CDN or React-warning `console.error` doesn't flake CI). Playwright is resolved via `createRequire` based at `clients/web/package.json` — a bare `import("playwright")` would resolve relative to `scripts/`, not the cwd, so it can't be reached that way (it only appears to work when an ancestor `node_modules` carries playwright, and fails in CI, which has none). The npm script's `cd clients/web` exists only so `npx playwright install chromium` finds the local playwright bin (a no-op when already installed). -- `smoke:web:app` (`scripts/smoke-web-app.mjs`, #1859) goes one step further again: `smoke:web:browser` stops at first paint and never connects to a server, so the Apps tab, the sandbox controller, and the UI-protocol bridge were unexercised by any smoke. This one boots the same prod `--web` server, spawns the `mcp-app-http.json` composable test server (the `mcp_app_demo` tool + its `mcp_app_demo_widget` UI resource), and drives the whole **connect → open app → widget ready** chain through a single deep-link navigate (`?serverUrl=…&autoConnect=&openApp=…&appArgs=…&autoOpen=`). The assertion is the `data-app-status="ready"` contract from [clients/web/README.md](clients/web/README.md) — the renderer reports `ready` only once the widget has loaded inside the sandbox iframe _and_ fired `notifications/initialized` back through the bridge, so one attribute covers the sandbox proxy being served, the UI resource loading, and the handshake completing. Two mechanics are load-bearing and easy to get wrong: the test server announces readiness on **stderr** (`console.error` in `server-composable.ts`), so both child streams are piped and scanned — watching stdout alone times out with an empty diagnostic; and its bound port is **not** the config's, because `createTestServerHttp` resolves through `findAvailablePort()`, which walks upward when the configured port is taken — so the smoke parses the announced URL rather than assuming `3130`. Both mechanics now live in `scripts/lib/announced-child.mjs` rather than in the smoke, so the failure path is testable: this smoke's happy path always receives the announcement, so nothing it could assert would prove that a child alive *through* the 30s timeout is still reachable by `shutdown()` — the case that orphaned a live server (#2000). The helper publishes the child via `onSpawn` before waiting, and `scripts/lib/announced-child.test.mjs` drives real `node -e` children (not spies) to assert it is published, still alive when the throw lands, and actually killable. Same reason `teardownWebServer` was extracted from `prod-web-server.mjs`'s `stop()`. **Scope note:** this runs against the repo build tree like every other smoke, so it would _not_ have caught #1859 itself (a packaging failure — the file is always present in-repo); `pack:verify` owns that dimension. It does carry a cheap structural pre-check that the proxy page exists at the path `sandbox-controller.ts` resolves, so a move/rename fails fast with a clear cause instead of an opaque render timeout. +- `smoke:web:app` (`scripts/smoke-web-app.mjs`, #1859) goes one step further again: `smoke:web:browser` stops at first paint and never connects to a server, so the Apps tab, the sandbox controller, and the UI-protocol bridge were unexercised by any smoke. This one boots the same prod `--web` server, spawns the `mcp-app-http.json` composable test server (the `mcp_app_demo` tool + its `mcp_app_demo_widget` UI resource), and drives the whole **connect → open app → widget ready** chain through a single deep-link navigate (`?serverUrl=…&autoConnect=&openApp=…&appArgs=…&autoOpen=`). The assertion is the `data-app-status="ready"` contract from [clients/web/README.md](clients/web/README.md) — the renderer reports `ready` only once the widget has loaded inside the sandbox iframe _and_ fired `notifications/initialized` back through the bridge, so one attribute covers the sandbox proxy being served, the UI resource loading, and the handshake completing. Two mechanics are load-bearing and easy to get wrong: the test server announces readiness on **stderr** (`console.error` in `server-composable.ts`), so both child streams are piped and scanned — watching stdout alone times out with an empty diagnostic; and its bound port is **not** the config's, because `createTestServerHttp` resolves through `findAvailablePort()`, which walks upward when the configured port is taken — so the smoke parses the announced URL rather than assuming `3130`. Both mechanics now live in `scripts/lib/announced-child.mjs` rather than in the smoke, so the failure path is testable: this smoke's happy path always receives the announcement, so nothing it could assert would prove that a child alive *through* the 30s timeout is still reachable by `shutdown()` — the case that orphaned a live server (#2000). The helper publishes the child via `onSpawn` before waiting, and `scripts/lib/announced-child.test.mjs` drives real `node -e` children (not spies) to assert it is published, still alive when the throw lands, and actually killable. Same reason `teardownWebServer` was extracted from `prod-web-server.mjs`'s `stop()`. **Scope note:** this runs against the repo build tree like every other smoke, so it would _not_ have caught #1859 itself (a packaging failure — the file is always present in-repo); `pack:verify` owns that dimension, and since #2003 it owns it *properly* — it drives this smoke's first phase against the installed tarball rather than only asserting the proxy page exists. The flow therefore lives in `scripts/lib/mcp-app-flow.mjs` (deep-link construction, the staged assertions, the Chromium/diagnostics plumbing) and is **shared, not copied**: two copies of the deep-link shape would drift, and a drifted link fails as a silent timeout rather than a mismatch. Keep both consumers — `pack:verify` is network-bound and local/release-only, so it does not run in `npm run ci`, where this smoke is the only thing exercising the App path at all, and the **`_meta.ui.domain` phase (#2056) is this smoke's alone**: the dedicated app origin is served by the same runner the packaging checks already cover, so driving it a second time from the tarball would cost a browser launch for no new packaging signal. It does carry a cheap structural pre-check that the proxy page exists at the path `sandbox-controller.ts` resolves, so a move/rename fails fast with a clear cause instead of an opaque render timeout. - `smoke:web:elicit` (`scripts/smoke-web-elicitation.mjs`, #1854) is the app-rendered **elicitation** counterpart of `smoke:web:app`: same prod `--web` server and the same deep-link connect, but it then calls `app_choose_option` from the Tools tab, waits for `[data-testid="app-elicitation"][data-app-elicitation-status="ready"]`, clicks a choice **inside the sandboxed app** (two `frameLocator` hops — the trusted sandbox proxy, then the untrusted app), and asserts the app's standard `ElicitResult` comes back in the *tool result*, i.e. that it reached the server rather than merely the host. It then repeats against `app-elicitation-native-http.json` — the same tool and app on a server that never advertised the nested MCP Apps `elicitation` capability — and asserts the **native** elicitation dialog takes it and no app modal is rendered. That second half is the more valuable one: the failure this feature can produce is not "the app doesn't render" but "an app renders when it should not have been offered one", which strands every user of a server that never opted in. Set `SMOKE_SCREENSHOT_DIR` to capture PNGs of the three states (used for PR proof); unset, it asserts only. Two mechanics worth knowing: the main-view tabs are a Mantine `SegmentedControl`, so there is no `role="tab"` — the clickable element is the sibling `label[for$="-Tools"]`; and the prompt string also appears in the (hidden) Protocol-tab payload, so the fallback assertion is scoped to the dialog rather than a bare text lookup. - **The build gate for the browser-externalized-builtin class (#1769)** is the earlier, more complete companion to `smoke:web:browser`. A Vite plugin in `clients/web/vite.config.ts` (logic in `clients/web/server/browser-externalized-builtin-gate.ts`, unit-tested) turns Vite 8's _browser-externalization warning_ (`Module "node:*" has been externalized for browser compatibility`) into a hard `vite build` error, so a Node built-in in the browser graph now **fails `npm run build` / `validate`** instead of shipping a `{}` stub. This catches **both** the _called-at-init_ case (which `smoke:web:browser` also catches, but later/at runtime) **and** the _imported-but-never-called_ case (the `{}` stub that is invisible to the runtime smoke "by design" — see above). Because rolldown **swallows a throw inside `onLog`** (the one hook where a thrown error doesn't abort — verified against vite@8.0.0), the plugin _records_ the warning in `onLog` and re-throws in `buildEnd`. There is **no stable log `code`**, so the gate keys off the documented message phrasing; `npm run verify:build-gate` (`scripts/verify-build-gate.mjs`, in `npm run ci` and the GitHub workflow) runs a real build with a `node:fs` probe forced into `src/main.tsx` and asserts the build fails via the gate — the only check that catches the message phrasing **drifting** in a future Vite bump and silently disabling the gate. The gate is scoped to `vite build` (`apply: 'build'`) — never `vite dev` or the vitest projects — **and** to the browser (`client`) environment (`applyToEnvironment`), so a future SSR/node environment built from this config isn't failed for a legitimate `node:*` import; the Node runner build (tsup, `build:runner`) is a separate config where built-ins are legitimate. `smoke:web:browser` stays as the runtime backstop for crashes the build can't reason about. diff --git a/Dockerfile b/Dockerfile index 24455398..84f40d29 100644 --- a/Dockerfile +++ b/Dockerfile @@ -32,11 +32,14 @@ ENV HOST=0.0.0.0 \ DANGEROUSLY_BIND_ALL_INTERFACES=true \ CLIENT_PORT=6274 \ MCP_SANDBOX_PORT=6275 \ + MCP_APP_ORIGIN_PORT=6278 \ MCP_AUTO_OPEN_ENABLED=false -# 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 +# 6275 is the MCP Apps sandbox and 6278 the dedicated app origin (#2056) — two +# further listeners the browser reaches DIRECTLY, so neither works through the +# 6274 publish alone. Both are only needed for the Apps tab (6278 only for an +# App declaring `_meta.ui.domain`), so they are EXPOSEd but publishing them is # optional — see the Docker section of the root README. -EXPOSE 6274 6275 +EXPOSE 6274 6275 6278 # 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 257c15b5..c8c15bf4 100644 --- a/README.md +++ b/README.md @@ -143,6 +143,7 @@ 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) | | `app-elicitation-http.json` **(legacy era)** | An MCP App rendering a form elicitation | [#1854](https://github.com/modelcontextprotocol/inspector/issues/1854) | +| `mcp-app-domain-http.json` **(legacy era)** | An MCP App asking for a dedicated origin (`_meta.ui.domain`) | [#2056](https://github.com/modelcontextprotocol/inspector/issues/2056) | | `modern-mrtr-http.json` | A single MRTR round-trip | — | | `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) | @@ -166,6 +167,16 @@ Open the Apps tab, select `mcp_app_demo`, give it a title and click **Open App** For the scripted version of the same flow (`--app-info` probe → deep link → rendered widget), see [Reviewing an MCP App](./docs/mcp-app-review.md). +#### An App's dedicated origin + +`mcp-app-domain-http.json` serves the same `mcp_app_demo` widget as above, with one addition: its UI resource declares `_meta.ui.domain`. Plain streamable-HTTP; connect with the **default (legacy)** protocol era. + +That field is how a server asks its host for a stable, dedicated origin. Without one, an App renders into a `srcdoc` frame sandboxed without `allow-same-origin`, so its document has an *opaque* origin and every request it makes carries `Origin: null` — which no CORS policy, OAuth callback, or API-key allowlist can admit ([#2056](https://github.com/modelcontextprotocol/inspector/issues/2056)). + +Open the Apps tab and run `mcp_app_demo`. The widget renders identically to `mcp-app-http.json` — the difference is not visual. Inspect the inner iframe in devtools: on this server it is served from `http://127.0.0.1:6278/app-document/` and `location.origin` is that real origin, where on `mcp-app-http.json` it is `about:srcdoc` with an origin of `null`. (The host is whatever the Inspector bound to — `127.0.0.1` by default, an *address* rather than the name `localhost`, for the reason `resolve-bind-host.ts` documents.) + +The spec makes `domain`'s format **host-dependent**, and the Inspector owns no domain infrastructure — so it reads any non-empty value as a *request* rather than an address, and answers with a real loopback origin of its own. See [MCP App dedicated origins](./clients/web/README.md#mcp-app-dedicated-origins-metauidomain) for the full contract, including what the one shared origin does and does not isolate, and how every failure falls back to the default render rather than blanking the app. + #### App-rendered form elicitations `app-elicitation-http.json` serves `app_choose_option` alongside the `choose_option_app` UI resource (`ui://demo/choose-option.html`). The tool sends a completely ordinary form `elicitation/create` — the only thing added is `_meta.ui.resourceUri` naming that app. Plain streamable-HTTP; connect with the **default (legacy)** protocol era. @@ -385,11 +396,12 @@ Each client self-validates from its own folder; the root scripts chain them. The | `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 three 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, and an **app-rendered elicitation** smoke (`smoke:web:elicit`) that drives one end to end — call the tool, answer inside the sandboxed app, see the app's `ElicitResult` reach the server — and then the same tool against a server that never advertised the capability, which must fall back to the native elicitation form. | | `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:bundle-externals` | Guards the must-not-bundle invariant (#2067): for each tsup-bundled client it reads the **built** `build/` output and fails if any package that must stay external was inlined anyway. Candidates are the union of the client's own `external` array **and the root manifest's `dependencies`** — the latter because #2067 was a *missing* `external` entry, which a self-referential check would have passed. Detection is via esbuild's `// ` module banners, so it covers both shapes — a separate `-HASH.js` chunk (what a dynamically `import()`ed CommonJS package produces) and a statically-imported package folded straight into `index.js`, which emits no chunk at all. A build with no banners fails as such rather than passing clean, so enabling `minify` cannot silently retire the check. `undici` was declared only in the root and `clients/cli` manifests, and tsup auto-externalizes only what the _nearest_ manifest declares, so the web and TUI bundles inlined 1.05MB of it — and CommonJS inlined into an ESM bundle throws `Dynamic require of "assert" is not supported` on first use, from a specifier no user-side install can satisfy. Reads the output rather than the config because those two disagreed for four releases. 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`, `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 two suites over shared `scripts/lib` helpers that no smoke can check itself: `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 — and `announced-child.test.mjs` — the spawn/readiness ownership helper (#2000), which drives real `node -e` children to prove a child that never announces is still published to the caller before the timeout throws, and so is reachable by teardown rather than orphaned. 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 two suites over shared `scripts/lib` helpers that no smoke can check itself: `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 — and `announced-child.test.mjs` — the spawn/readiness ownership helper (#2000), which drives real `node -e` children to prove a child that never announces is still published to the caller before the timeout throws, and so is reachable by teardown rather than orphaned. A third, `mcp-app-flow.test.mjs`, covers the shared MCP Apps flow (#2003) — the deep link's two CSRF gates and `appArgs` encoding, plus `driveAppFlow`'s failure branches against a stand-in page, all of which are dead code from the happy-path smokes' point of view and would otherwise surface only as opaque timeouts. 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 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 ci` | **Mandatory pre-push command.** `validate` → `coverage` → `verify:build-gate` → `verify:bundle-externals` → `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. @@ -410,13 +422,13 @@ The root `package.json` `"files"` allowlist is the source of truth for the tarba - **No source maps.** The client bundlers set `sourcemap: false` (`clients/{cli,tui}/tsup.config.ts`, `clients/web/tsup.runner.config.ts`); Vite and the launcher's `tsc` already emit none. Maps are ~half the unpacked size and aren't needed at runtime — debug via `npm run dev` on the source. - **`clients/web/build` ships via `clients/web/.npmignore`.** `clients/web/.gitignore` lists `build/`, and npm's packlist honors that nested `.gitignore` over the root `"files"` allowlist — so the prod web-server runner was silently missing from the tarball while `clients/web/dist` slipped through (its `.gitignore` only lists `dist-ssr`). `clients/web/.npmignore` overrides the `.gitignore` for publishing so both `build/` (runner) and `dist/` (SPA) ship. The other clients don't need this — none ship a nested `.gitignore`. -- **`clients/web/static` ships the MCP Apps sandbox proxy.** `clients/web/static/sandbox_proxy.html` is a committed source file (not a build artifact), read from disk at runtime by `clients/web/server/sandbox-controller.ts` as `/../static/sandbox_proxy.html`. It was missing from the root `"files"` allowlist entirely, so every published build failed the Apps tab with **"Sandbox not loaded"** ([#1859](https://github.com/modelcontextprotocol/inspector/issues/1859)) while working fine in the repo. Because the path is resolved _relative to_ `clients/web/build`, the directory must ship at that exact location — `pack:verify` asserts both the tarball entry and the installed-on-disk path. +- **`clients/web/static` ships the MCP Apps sandbox proxy.** `clients/web/static/sandbox_proxy.html` is a committed source file (not a build artifact), read from disk at runtime by `clients/web/server/sandbox-controller.ts` as `/../static/sandbox_proxy.html`. It was missing from the root `"files"` allowlist entirely, so every published build failed the Apps tab with **"Sandbox not loaded"** ([#1859](https://github.com/modelcontextprotocol/inspector/issues/1859)) while working fine in the repo. Because the path is resolved _relative to_ `clients/web/build`, the directory must ship at that exact location — `pack:verify` asserts the tarball entry, the installed-on-disk path, and (since [#2003](https://github.com/modelcontextprotocol/inspector/issues/2003)) that a widget actually *loads through* it on the installed bin. Presence and reachability are different properties: a rename with a stale reader ships a file that is there and unusable. - **A dependency that renders React is bundled, not externalized.** An externalized package resolves its own `react` from wherever npm placed **it** in the consumer's tree, which is not necessarily where the bundle resolves ours — npm places a package beside a React satisfying _its_ peer range, and those ranges are looser than ours. `ink-form` and `ink-scroll-view` declare `">=18"`, so a project holding React 18 satisfies them and gets them hoisted while the Inspector's React 19 nests underneath: two React copies, and the TUI dies with `TypeError: Cannot read properties of null (reading 'useState')` the moment a tool test form or a scroll view mounts ([#1952](https://github.com/modelcontextprotocol/inspector/issues/1952)). Both are therefore inlined by `clients/tui/tsup.config.ts` and are **not** root dependencies: the tarball ships their code inside `clients/tui/build/index.js` rather than having consumers install them. Bundling also pins their transitive deps to what this repo's install resolved (notably `ink-select-input@6` via `overrides`, which npm ignores for a package installed as a dependency). **`ink` is the one exception, on cost:** bundling it works but adds ~1.4 MB (`react-reconciler` and `yoga-layout` come along, plus a `createRequire` banner for the inlined CJS), so it stays external — _not_ because its `">=19"` peer makes it safe, which it does not. What keeps that tolerable is the root `react` range: `"^19.0.0"` is deliberately open to the whole major so npm can dedupe our React with whatever React 19 a consumer pins, leaving an external `ink` on the same copy the bundle uses. **Narrowing that range reopens the bug for the renderer itself** — `clients/tui/__tests__/tsupConfig.test.ts` pins it to `ink`'s peer floor, and guards the rest of the split; see the [TUI README](./clients/tui/README.md#bundling-react-rendering-dependencies-must-be-inlined-1952). - **A single version number, read from the root `package.json`.** The Inspector ships as one package with one version, so only the **root** `package.json` carries a `version` — the four `clients/*/package.json`s deliberately have none. Every Node client (CLI, TUI, and the web backend) resolves the version through the shared `readInspectorVersion()` reader in `core/node/version.ts`, which walks up to the root manifest (always present in the tarball). No client `package.json` is read at runtime, so none needs to ship. The web **browser** can't read the filesystem; it gets its version from the backend via `GET /api/config` (see [#1639](https://github.com/modelcontextprotocol/inspector/issues/1639)). ### `npm run pack:verify` — publish smoke against the real tarball -The `smoke:*` scripts run against the in-repo build tree, which is **not** the published package. `npm run pack:verify` (`scripts/pack-and-verify.mjs`) closes that gap: it builds, `npm pack`s the publishable tarball (asserting no source maps ship and that the runtime-required files are present), installs the tarball into a **clean throwaway consumer** — a fresh temp directory where it runs a real `npm install ` (pulls runtime deps, runs `postinstall`), exactly as `npx @modelcontextprotocol/inspector` would — and drives the installed `mcp-inspector` bin end to end: `--help` dispatch, a real `--cli tools/list` over stdio, and a prod `--web` boot that must serve `/` from the shipped `dist`. It catches "works in `--dev`, breaks under `npx …`" path/packaging failures. It requires network access (the install pulls deps), so it is a local / release check, **not** part of the fast `validate`/`ci` loop. +The `smoke:*` scripts run against the in-repo build tree, which is **not** the published package. `npm run pack:verify` (`scripts/pack-and-verify.mjs`) closes that gap: it builds, `npm pack`s the publishable tarball (asserting no source maps ship and that the runtime-required files are present), installs the tarball into a **clean throwaway consumer** — a fresh temp directory where it runs a real `npm install ` (pulls runtime deps, runs `postinstall`), exactly as `npx @modelcontextprotocol/inspector` would — and drives the installed `mcp-inspector` bin end to end: `--help` dispatch, a real `--cli tools/list` over stdio, a prod `--web` boot that must serve `/` from the shipped `dist`, and — riding that same boot — an **MCP App rendered in headless Chromium** through the shipped sandbox proxy, connect → open app → `data-app-status="ready"` ([#2003](https://github.com/modelcontextprotocol/inspector/issues/2003)). That last step shares its flow with `smoke:web:app` via `scripts/lib/mcp-app-flow.mjs` rather than copying it, since the deep-link shape is the part that rots silently; the *client* comes from the install while the App test server stays a repo fixture. It catches "works in `--dev`, breaks under `npx …`" path/packaging failures. It requires network access (the install pulls deps), so it is a local / release check, **not** part of the fast `validate`/`ci` loop. ### Cutting a release @@ -483,7 +495,14 @@ 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. +**And `6278` if your app declares `_meta.ui.domain`.** That is the spec field a server uses to ask its host for a stable, dedicated origin — without one the app runs at an opaque origin and its requests carry `Origin: null`, which no CORS / OAuth-callback / API-key allowlist can admit. The Inspector answers the request with a real loopback origin on a third listener, `MCP_APP_ORIGIN_PORT` (default `6278`); apps that declare no `domain` never touch it. **Publish it if you use one** — this is the one failure that does not fall back: the backend publishes fine (its listener bound inside the container), so it hands the browser a URL on a port the browser cannot reach, and that app's frame stays **blank**. The cross-origin navigation failure is not observable from the page, so there is no opaque-origin fallback and no console warning here; those cover the failures the *backend* can see (no listener, a port that never bound, an older backend). See [MCP App dedicated origins](./clients/web/README.md#mcp-app-dedicated-origins-metauidomain) for the host-specific contract and its isolation trade-offs. + +```bash +docker run --rm -p 127.0.0.1:6274:6274 -p 127.0.0.1:6275:6275 -p 127.0.0.1:6278:6278 \ + ghcr.io/modelcontextprotocol/inspector +``` + +Publish each 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. The same holds for the app origin: the URL a published app document is served from is built from the port the container bound, so remap with `-e MCP_APP_ORIGIN_PORT=9001 -p 127.0.0.1:9001:9001` rather than with `-p` alone. **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. diff --git a/clients/cli/README.md b/clients/cli/README.md index d5c033df..e0572746 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -88,9 +88,15 @@ The file is the only durable way to give a run its roots: there is no roots flag ### HTTP proxy support -Connections to remote HTTP/SSE servers honor the conventional proxy environment variables: `HTTPS_PROXY` / `HTTP_PROXY` (and their lowercase forms) select the proxy, and `NO_PROXY` exempts hosts. This applies to the Node transport shared by the CLI and the web backend — no inspector-specific flag is needed. When a proxy variable is set, outbound requests are routed through undici's `EnvHttpProxyAgent`. +Connections to remote HTTP/SSE servers honor the conventional proxy environment variables: `HTTPS_PROXY` / `HTTP_PROXY` (and their lowercase forms) select the proxy, and `NO_PROXY` exempts hosts. This applies to every Node client — CLI, TUI, and the web backend — with no inspector-specific flag. It also covers **OAuth discovery and token requests**, which run through the same fetch. -Proxy routing is powered by the [`undici`](https://www.npmjs.com/package/undici) package (`^8.5.0`, which requires Node `>= 22.19.0` — the inspector's supported floor). It is imported lazily only when a proxy variable is set, so runs without a proxy configured pay no cost. +Proxy routing is powered by the [`undici`](https://www.npmjs.com/package/undici) package (declared in the **root** manifest only; `^8.x` requires Node `>= 22.19.0`, the Inspector's supported floor). It is imported lazily on the first request and only when a proxy variable is set, so runs without a proxy configured pay no cost. + +**Both halves of the pair come from userland undici — its `fetch` _and_ its `EnvHttpProxyAgent` ([#2067](https://github.com/modelcontextprotocol/inspector/issues/2067)).** The obvious-looking alternative is to hand the agent to Node's built-in `fetch` as a `dispatcher`, which is what v2.0.0–2.3.0 did. That couples two _different copies_ of undici at the dispatcher handler interface, and that interface is not stable across majors: Node 22 embeds undici 6, Node 24 embeds 7, Node 26 embeds 8. A userland undici 8 agent handed a built-in undici 7 handler is rejected outright — `fetch failed: invalid onRequestStart method` — and the request never leaves the process. Keeping both sides inside one copy is what makes proxying work unchanged from the Node 22.19 floor through Node 26. (Node's own `NODE_USE_ENV_PROXY` is not a substitute: it is unsupported at our 22.19 engine floor.) + +Because undici's `Response` is a different class from `globalThis.Response`, the proxied fetch re-wraps each response as a genuine global `Response`, streaming preserved. Without that, `res instanceof Response` is `false` for callers that test it — including the MCP SDK, whose OAuth error formatter would degrade every message to `Raw body: [object Response]`. + +`undici` is declared **only** in the root `package.json`, and every client's tsup config lists it as `external`. Both halves matter: tsup auto-externalizes what the _nearest_ manifest declares, so without the explicit entry the web and TUI bundles inlined it — and a CommonJS package inlined into an ESM bundle throws `Dynamic require of "assert" is not supported` the first time it is used. `npm run verify:bundle-externals` is the durable guard. ## Options @@ -110,15 +116,15 @@ Options that specify the MCP server (catalog/config file, ad-hoc command/URL, en | `--prompt-name ` | Prompt name (for `prompts/get`). | | `--prompt-args ` | Prompt arguments; repeat for multiple. | | `--log-level ` | Logging level for `logging/setLevel` (e.g. `debug`, `info`). | -| `--metadata ` | General metadata (key=value); applied to all methods. | -| `--tool-metadata ` | Tool-specific metadata for `tools/call`. | +| `--metadata ` | General `_meta` entries (key=value); applied to all methods. The value is JSON-parsed when it parses, so `trace={"id":"abc"}` sends a real object and `n=3` a number; anything that is not valid JSON is sent as the literal string. Merged **over** the server's persisted `metadata` from `mcp.json`, which is sent on every request without this flag (#2093). | +| `--tool-metadata ` | Tool-specific `_meta` entries for `tools/call`. Same JSON-parsed value handling as `--metadata`. | | `--connect-timeout ` | Connection timeout in ms. Defaults to `15000` for ad-hoc `--server-url`/target runs (so a black-holed host fails fast) and to the file-level timeout for `--catalog`/`--config` runs. `0` disables the timeout. | | `--app-info` | Probe a tool's MCP App UI metadata without invoking it. With `--method tools/call --tool-name `: prints one JSON line (`hasApp`, `resourceUri`, `csp`, `permissions`, `domain`, …) and exits `0` if the tool has an app or `2` (`no_app`) if not. With `--method tools/list`: emits NDJSON — one app-info line per tool over a single connection. | | `--format ` | Output format. `text` (default) pretty-prints the result. `json` emits a single JSON object on stdout (`{ "result": … }`, plus `{ "appInfo": … }` as a sibling key for App tools) with no banners, so the whole output pipes cleanly into `jq`. | | `--relogin` | Delete stored OAuth for this server URL from the shared store before connect; interactive login still only runs if the server requires auth. Requires an HTTP/SSE URL (rejected for stdio). Conflicts with `--stored-auth-only` / `--use-stored-auth` / `--wait-for-auth` / catalog short-circuits. | | `--stored-auth-only` | **CI / non-interactive safe:** never start interactive OAuth / step-up (and never auto-open a browser); use the shared store if present, otherwise fail immediately with `auth_required`. Prefer this over a bare pipe/CI run that would otherwise attempt interactive login. | -`servers/show` redacts secret-bearing fields (`env` values, sensitive headers / `settings.metadata` keys, `requestInit` / `eventSourceInit` headers, `oauthClientSecret`). It does **not** scrub credentials embedded in a server `url` (userinfo or query tokens) or in stdio `args` — treat `detail` / raw URL fields as potentially sensitive before pasting into issues. +`servers/show` redacts secret-bearing fields (`env` values, sensitive headers, sensitive `settings.metadata` keys whose whole value is replaced whether or not it is structured, `requestInit` / `eventSourceInit` headers, `oauthClientSecret`). It does **not** scrub credentials embedded in a server `url` (userinfo or query tokens) or in stdio `args` — treat `detail` / raw URL fields as potentially sensitive before pasting into issues. #### App probing (`--app-info`) and machine-readable output (`--format json`) diff --git a/clients/cli/__tests__/cli.test.ts b/clients/cli/__tests__/cli.test.ts index d50cd861..020f1446 100644 --- a/clients/cli/__tests__/cli.test.ts +++ b/clients/cli/__tests__/cli.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, vi } from "vitest"; +import { afterAll, beforeAll, describe, it, expect, vi } from "vitest"; import { mkdtempSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -24,8 +24,44 @@ import { createTestServerInfo, } from "@modelcontextprotocol/inspector-test-server"; import type { MCPServerConfig } from "@inspector/core/mcp/index.js"; +import { createServer, request } from "node:http"; describe("CLI Tests", () => { + /** + * Proxy variables are cleared for the WHOLE file, not just the proxy tests. + * + * `createProxyFetch`'s `EnvHttpProxyAgent` is memoized process-wide and captures + * its proxy URLs when it is constructed (only `NO_PROXY` is re-read per + * request). So on a machine with an ambient `HTTP_PROXY`, any earlier test that + * makes a real connection would build that agent against the ambient proxy, and + * a later test setting `HTTP_PROXY` to its own server would be talking to an + * agent that is not listening. Clearing up front means nothing can build the + * singleton before the proxy tests do. + */ + const AMBIENT_PROXY_VARS = [ + "HTTP_PROXY", + "http_proxy", + "HTTPS_PROXY", + "https_proxy", + "NO_PROXY", + "no_proxy", + ] as const; + const ambientProxyEnv: Record = {}; + + beforeAll(() => { + for (const name of AMBIENT_PROXY_VARS) { + ambientProxyEnv[name] = process.env[name]; + delete process.env[name]; + } + }); + + afterAll(() => { + for (const name of AMBIENT_PROXY_VARS) { + const previous = ambientProxyEnv[name]; + if (previous === undefined) delete process.env[name]; + else process.env[name] = previous; + } + }); describe("Basic CLI Mode", () => { it("should execute tools/list successfully", async () => { const { command, args } = getTestMcpServerCommand(); @@ -326,6 +362,75 @@ describe("CLI Tests", () => { }); }); + describe("HTTP proxy (#2067)", () => { + it("routes the MCP connection through HTTP_PROXY (#2067)", async () => { + // Proxy support is one line in cli.ts — `fetch: createProxyFetch()` on the + // environment — and it is what puts the proxy at the BOTTOM of the fetch + // stack, where InspectorClient's own wrappers compose over it rather than + // discarding it. `InspectorClient` sets `fetchFn` unconditionally, so the + // transport-level fallback never fires for this client; delete that line + // and CLI proxy support disappears with every core test still green. + // + // Asserted behaviorally, through a real forwarding proxy, rather than by + // inspecting the environment object: the bug this replaces was a runtime + // dispatch failure that no structural assertion would have caught. + const server = createTestServerHttp({ + serverInfo: createTestServerInfo(), + tools: [createEchoTool()], + }); + const seen: string[] = []; + const proxy = createServer((req, res) => { + seen.push(req.url ?? ""); + const target = new URL(req.url ?? ""); + const upstream = request( + { + host: target.hostname, + port: target.port, + path: target.pathname + target.search, + method: req.method, + headers: req.headers, + }, + (up) => { + res.writeHead(up.statusCode ?? 502, up.headers); + up.pipe(res); + }, + ); + upstream.on("error", () => { + res.writeHead(502); + res.end(); + }); + req.pipe(upstream); + }); + try { + await server.start(); + await new Promise((r) => proxy.listen(0, "127.0.0.1", () => r())); + const addr = proxy.address(); + const port = typeof addr === "object" && addr !== null ? addr.port : 0; + process.env.HTTP_PROXY = `http://127.0.0.1:${port}`; + + const result = await runCli([ + server.url, + "--cli", + "--method", + "tools/list", + ]); + + expectCliSuccess(result); + expect(JSON.stringify(expectValidJson(result))).toContain("echo"); + // The point of the test: it did not go direct. + expect(seen.length).toBeGreaterThan(0); + expect(seen.every((u) => u.startsWith(server.url))).toBe(true); + } finally { + // Only this test's own variable is cleared here. The ambient ones are + // restored by the file-level afterAll, so the memoized agent can never + // be left bound to this now-closed proxy for a later test. + delete process.env.HTTP_PROXY; + await new Promise((r) => proxy.close(() => r())); + await server.stop(); + } + }); + }); + describe("Roots capability (#1797)", () => { it("answers a server's roots/list instead of -32601", async () => { // The CLI used to omit `roots` when constructing its InspectorClient, so diff --git a/clients/cli/__tests__/helpers/fixtures.ts b/clients/cli/__tests__/helpers/fixtures.ts index abb25f51..ce16e6bb 100644 --- a/clients/cli/__tests__/helpers/fixtures.ts +++ b/clients/cli/__tests__/helpers/fixtures.ts @@ -3,7 +3,7 @@ import * as path from "path"; import * as os from "os"; import * as crypto from "crypto"; import { getTestMcpServerCommand } from "@modelcontextprotocol/inspector-test-server"; -import type { MCPServerConfig } from "@inspector/core/mcp/index.js"; +import type { StoredMCPServer } from "@inspector/core/mcp/types.js"; /** * Sentinel value for tests that don't need a real server @@ -61,10 +61,16 @@ function cleanupTempDir(dir: string) { } /** - * Create a test config file + * Create a test config file. + * + * Entries are `StoredMCPServer`, not bare `MCPServerConfig` — the on-disk + * `mcp.json` shape carries the Inspector-specific per-server keys (`metadata`, + * `headers`, `roots`, `protocolEra`, …) directly alongside `type`/`url`, and a + * fixture that cannot express them cannot cover the settings that reach the + * client from the catalog (#2093). */ export function createTestConfig(config: { - mcpServers: Record; + mcpServers: Record; }): string { const tempDir = createTempDir("mcp-inspector-config-"); const configPath = path.join(tempDir, "config.json"); diff --git a/clients/cli/__tests__/helpers/oauth-test-fakes.ts b/clients/cli/__tests__/helpers/oauth-test-fakes.ts index b53e1570..8c5e91f9 100644 --- a/clients/cli/__tests__/helpers/oauth-test-fakes.ts +++ b/clients/cli/__tests__/helpers/oauth-test-fakes.ts @@ -63,7 +63,7 @@ export function makeFakeServerSettings( ): InspectorServerSettings { return { headers: [], - metadata: [], + metadata: {}, env: [], connectionTimeout: 0, requestTimeout: 0, diff --git a/clients/cli/__tests__/metadata.test.ts b/clients/cli/__tests__/metadata.test.ts index b5867d50..ab05d348 100644 --- a/clients/cli/__tests__/metadata.test.ts +++ b/clients/cli/__tests__/metadata.test.ts @@ -1,5 +1,6 @@ import { describe, it, expect } from "vitest"; import { runCli } from "./helpers/cli-runner.js"; +import type { StrictJsonObject } from "@inspector/core/json/jsonUtils.js"; import { expectCliSuccess, expectCliFailure, @@ -11,7 +12,11 @@ import { createAddTool, createTestServerInfo, } from "@modelcontextprotocol/inspector-test-server"; -import { NO_SERVER_SENTINEL } from "./helpers/fixtures.js"; +import { + NO_SERVER_SENTINEL, + createTestConfig, + deleteConfigFile, +} from "./helpers/fixtures.js"; describe("Metadata Tests", () => { describe("General Metadata", () => { @@ -373,16 +378,17 @@ describe("Metadata Tests", () => { expectCliSuccess(result); - // Validate metadata values are sent as strings + // A JSON-parseable value goes out as that JSON type, not as its + // string spelling (#1910) — `_meta` takes any JSON. const recordedRequests = server.getRecordedRequests(); const toolsListRequest = recordedRequests.find( (r) => r.method === "tools/list", ); expect(toolsListRequest).toBeDefined(); expect(toolsListRequest?.metadata).toEqual({ - integer_value: "42", - decimal_value: "3.14159", - negative_value: "-10", + integer_value: 42, + decimal_value: 3.14159, + negative_value: -10, }); } finally { await server.stop(); @@ -428,7 +434,7 @@ describe("Metadata Tests", () => { } }); - it("JSON.stringifies object/array metadata (not String → [object Object])", async () => { + it("sends object/array/boolean metadata as real JSON, not stringified (#1910)", async () => { const server = createTestServerHttp({ serverInfo: createTestServerInfo(), tools: [createEchoTool()], @@ -456,9 +462,9 @@ describe("Metadata Tests", () => { (r) => r.method === "tools/list", ); expect(toolsListRequest?.metadata).toEqual({ - nested: '{"key":"value"}', - list: "[1,2,3]", - flag: "true", + nested: { key: "value" }, + list: [1, 2, 3], + flag: true, }); } finally { await server.stop(); @@ -819,7 +825,7 @@ describe("Metadata Tests", () => { ); expect(toolsListRequest).toBeDefined(); expect(toolsListRequest?.metadata).toEqual({ - integration_test: "true", + integration_test: true, test_phase: "all_methods", }); } finally { @@ -866,8 +872,8 @@ describe("Metadata Tests", () => { ); expect(toolCallRequest).toBeDefined(); expect(toolCallRequest?.metadata).toEqual({ - session_id: "12345", - user_id: "67890", + session_id: 12345, + user_id: 67890, timestamp: "2024-01-01T00:00:00Z", request_id: "req-abc-123", tool_session: "session-xyz-789", @@ -879,6 +885,65 @@ describe("Metadata Tests", () => { } }); + it.each([ + ["an overflowing literal", "n=1e400"], + ["a negative overflow", "n=-1e400"], + ["one nested in an object", 'o={"a":1e400}'], + ])("rejects %s rather than sending null", async (_label, pair) => { + // `JSON.parse` accepts these and yields ±Infinity, which + // `JSON.stringify` writes as `null` — so accepting the flag would + // transmit a value the user did not ask for. + const server = createTestServerHttp({ + serverInfo: createTestServerInfo(), + tools: [createEchoTool()], + }); + try { + await server.start(); + const result = await runCli([ + server.url, + "--cli", + "--method", + "tools/list", + "--metadata", + pair, + "--transport", + "http", + ]); + expect(result.exitCode).not.toBe(0); + expect(`${result.stderr}${result.stdout}`).toMatch(/finite/i); + } finally { + await server.stop(); + } + }); + + it("names the key but never the value when rejecting", async () => { + // The pair can carry a credential, and this message reaches stderr and + // CI logs. + const server = createTestServerHttp({ + serverInfo: createTestServerInfo(), + tools: [createEchoTool()], + }); + try { + await server.start(); + const result = await runCli([ + server.url, + "--cli", + "--method", + "tools/list", + "--metadata", + 'credentials={"accessToken":"sk-live-nope","n":1e400}', + "--transport", + "http", + ]); + const output = `${result.stderr}${result.stdout}`; + expect(result.exitCode).not.toBe(0); + expect(output).toContain("credentials"); + expect(output).not.toContain("sk-live"); + } finally { + await server.stop(); + } + }); + it("should handle metadata parsing validation", async () => { const server = createTestServerHttp({ serverInfo: createTestServerInfo(), @@ -919,8 +984,8 @@ describe("Metadata Tests", () => { expect(toolCallRequest).toBeDefined(); expect(toolCallRequest?.metadata).toEqual({ valid_key: "valid_value", - numeric_key: "123", - boolean_key: "true", + numeric_key: 123, + boolean_key: true, json_key: '\'{"test":"value"}\'', // Single quotes are preserved special_key: "!@#$%^&*()", unicode_key: "🚀🎉✨", @@ -968,4 +1033,143 @@ describe("Metadata Tests", () => { } }); }); + describe("Per-server metadata from mcp.json (#2093)", () => { + /** + * Write a one-server catalog pointing at `url`, carrying `metadata` as the + * on-disk per-server key. Returns the catalog path; the caller deletes it. + */ + function writeCatalogWithMetadata( + url: string, + metadata: StrictJsonObject, + ): string { + return createTestConfig({ + mcpServers: { + "meta-server": { + type: "streamable-http", + url, + metadata, + }, + }, + }); + } + + it("applies a server's persisted metadata to every request", async () => { + // The setting belongs to the server, not to the client that reads it — + // web and the TUI already honored it, and the CLI silently did not, + // because `InspectorClient` reads `defaultMetadata` rather than falling + // back to `serverSettings.metadata`. + const server = createTestServerHttp({ + serverInfo: createTestServerInfo(), + tools: [createEchoTool()], + }); + let catalogPath: string | undefined; + + try { + await server.start(); + catalogPath = writeCatalogWithMetadata(server.url, { tenant: "acme" }); + + const result = await runCli([ + "--catalog", + catalogPath, + "--server", + "meta-server", + "--cli", + "--method", + "tools/list", + ]); + + expectCliSuccess(result); + expect(expectValidJson(result)).toHaveProperty("tools"); + + const toolsListRequest = server + .getRecordedRequests() + .find((r) => r.method === "tools/list"); + expect(toolsListRequest).toBeDefined(); + expect(toolsListRequest?.metadata).toEqual({ tenant: "acme" }); + } finally { + if (catalogPath) deleteConfigFile(catalogPath); + await server.stop(); + } + }); + + it("merges --metadata over the persisted defaults", async () => { + // `--metadata` is per-invocation and stays that way: non-colliding keys + // merge with the catalog's, and a colliding one wins (call-time keys + // override defaults in `InspectorClient.mergeMeta`). + const server = createTestServerHttp({ + serverInfo: createTestServerInfo(), + tools: [createEchoTool()], + }); + let catalogPath: string | undefined; + + try { + await server.start(); + catalogPath = writeCatalogWithMetadata(server.url, { + tenant: "acme", + region: "eu", + }); + + const result = await runCli([ + "--catalog", + catalogPath, + "--server", + "meta-server", + "--cli", + "--method", + "tools/list", + "--metadata", + "tenant=override", + ]); + + expectCliSuccess(result); + + const toolsListRequest = server + .getRecordedRequests() + .find((r) => r.method === "tools/list"); + expect(toolsListRequest?.metadata).toEqual({ + tenant: "override", + region: "eu", + }); + } finally { + if (catalogPath) deleteConfigFile(catalogPath); + await server.stop(); + } + }); + + it("sends no _meta when the persisted metadata is empty", async () => { + // `{}` means "no defaults" — an empty map must not put a bare `_meta` on + // the wire, matching what the option-less CLI has always sent. + const server = createTestServerHttp({ + serverInfo: createTestServerInfo(), + tools: [createEchoTool()], + }); + let catalogPath: string | undefined; + + try { + await server.start(); + catalogPath = writeCatalogWithMetadata(server.url, {}); + + const result = await runCli([ + "--catalog", + catalogPath, + "--server", + "meta-server", + "--cli", + "--method", + "tools/list", + ]); + + expectCliSuccess(result); + + const toolsListRequest = server + .getRecordedRequests() + .find((r) => r.method === "tools/list"); + expect(toolsListRequest).toBeDefined(); + expect(toolsListRequest?.metadata).toBeUndefined(); + } finally { + if (catalogPath) deleteConfigFile(catalogPath); + await server.stop(); + } + }); + }); }); diff --git a/clients/cli/__tests__/method-types.test.ts b/clients/cli/__tests__/method-types.test.ts index 42c71025..85230043 100644 --- a/clients/cli/__tests__/method-types.test.ts +++ b/clients/cli/__tests__/method-types.test.ts @@ -1,7 +1,6 @@ import { describe, it, expect } from "vitest"; import { isOneShotMethod, - metaValueToString, ONE_SHOT_METHODS, SESSION_RPC_METHODS, } from "../src/handlers/method-types.js"; @@ -26,11 +25,3 @@ describe("ONE_SHOT_METHODS", () => { expect(ONE_SHOT_METHODS).not.toContain("logging/tail"); }); }); - -describe("metaValueToString", () => { - it("passes strings through and JSON-encodes structured values", () => { - expect(metaValueToString("plain")).toBe("plain"); - expect(metaValueToString({ a: 1 })).toBe('{"a":1}'); - expect(metaValueToString([1, 2])).toBe("[1,2]"); - }); -}); diff --git a/clients/cli/__tests__/programmatic-ergonomics.test.ts b/clients/cli/__tests__/programmatic-ergonomics.test.ts index 1c6fdee7..88f42f10 100644 --- a/clients/cli/__tests__/programmatic-ergonomics.test.ts +++ b/clients/cli/__tests__/programmatic-ergonomics.test.ts @@ -12,7 +12,7 @@ import { describe("withConnectTimeout", () => { const baseSettings: InspectorServerSettings = { headers: [{ key: "X-Test", value: "1" }], - metadata: [], + metadata: {}, env: [], connectionTimeout: 42, requestTimeout: 0, diff --git a/clients/cli/__tests__/servers-list.test.ts b/clients/cli/__tests__/servers-list.test.ts index 8ac7b599..daa1c444 100644 --- a/clients/cli/__tests__/servers-list.test.ts +++ b/clients/cli/__tests__/servers-list.test.ts @@ -219,10 +219,23 @@ describe("showServerEntry / servers/show", () => { { key: "Authorization", value: "Bearer x" }, { key: "X-Custom", value: "ok" }, ], - metadata: [ - { key: "Authorization", value: "Bearer meta" }, - { key: "X-Custom", value: "ok" }, - ], + metadata: { + Authorization: "Bearer meta", + "X-Custom": "ok", + // A structured value under a sensitive key: the redaction must replace + // the whole value, not walk into it (#1910). + "X-Api-Key": { primary: "sk-live-1", fallback: "sk-live-2" }, + nested: { keep: true }, + // A secret buried under a non-sensitive key. A top-level-only check + // would print `accessToken` in full. + trace: { + id: "t-1", + accessToken: "sk-live-3", + deeper: { refresh_token: "sk-live-4", ok: 1 }, + }, + // Objects inside an array are reached too. + attempts: [{ password: "hunter2" }, { attempt: 2 }], + }, env: [ { key: "TOKEN", value: "secret" }, { key: "", value: "still-secret" }, @@ -242,10 +255,20 @@ describe("showServerEntry / servers/show", () => { { key: "Authorization", value: "[redacted]" }, { key: "X-Custom", value: "ok" }, ]); - expect(sanitized.metadata).toEqual([ - { key: "Authorization", value: "[redacted]" }, - { key: "X-Custom", value: "ok" }, - ]); + expect(sanitized.metadata).toEqual({ + Authorization: "[redacted]", + "X-Custom": "ok", + "X-Api-Key": "[redacted]", + // A structured value under a non-sensitive key survives intact... + nested: { keep: true }, + // ...but a sensitive key nested inside one does not. + trace: { + id: "t-1", + accessToken: "[redacted]", + deeper: { refresh_token: "[redacted]", ok: 1 }, + }, + attempts: [{ password: "[redacted]" }, { attempt: 2 }], + }); expect(sanitized.env).toEqual([ { key: "TOKEN", value: "[redacted]" }, { key: "", value: "[redacted]" }, diff --git a/clients/cli/__tests__/stored-auth.test.ts b/clients/cli/__tests__/stored-auth.test.ts index 4a45ec33..1b9b5768 100644 --- a/clients/cli/__tests__/stored-auth.test.ts +++ b/clients/cli/__tests__/stored-auth.test.ts @@ -554,6 +554,7 @@ describe("--print-handoff", () => { MCP_INSPECTOR_API_TOKEN: "tok123", CLIENT_PORT: "16274", MCP_SANDBOX_PORT: "16275", + MCP_APP_ORIGIN_PORT: "16278", MCP_STORAGE_DIR: "/tmp/inspector-storage", MCP_INSPECTOR_OAUTH_STATE_PATH: "", }, @@ -574,6 +575,9 @@ describe("--print-handoff", () => { expect(out.deepLink).toContain("transport=http"); expect(out.portForwardCmd).toContain("--tcp 16274:16274"); expect(out.portForwardCmd).toContain("--tcp 16275:16275"); + // The dedicated app origin (#2056) forwards with the other two — an app + // declaring `_meta.ui.domain` is unreachable without it. + expect(out.portForwardCmd).toContain("--tcp 16278:16278"); expect(out.oauthStatePath).toBe( join("/tmp/inspector-storage", "oauth.json"), ); diff --git a/clients/cli/package-lock.json b/clients/cli/package-lock.json index 30a14836..2e5c05c4 100644 --- a/clients/cli/package-lock.json +++ b/clients/cli/package-lock.json @@ -14,7 +14,6 @@ "commander": "^13.1.0", "open": "^10.2.0", "pino": "^9.14.0", - "undici": "^8.9.0", "zod": "^4.4.3" }, "bin": { @@ -4440,15 +4439,6 @@ "dev": true, "license": "MIT" }, - "node_modules/undici": { - "version": "8.9.0", - "resolved": "https://registry.npmjs.org/undici/-/undici-8.9.0.tgz", - "integrity": "sha512-aWZpUj7XoGonMClx4gdDRfgBjqeA+F473aDmROQQbM9n6PRfK/u1q/a0X4wMTgcHfT8H6fpbt98PFuDUwFg2YA==", - "license": "MIT", - "engines": { - "node": ">=22.19.0" - } - }, "node_modules/undici-types": { "version": "7.18.2", "resolved": "https://registry.npmjs.org/undici-types/-/undici-types-7.18.2.tgz", diff --git a/clients/cli/package.json b/clients/cli/package.json index f77ecaf0..31ac096c 100644 --- a/clients/cli/package.json +++ b/clients/cli/package.json @@ -38,7 +38,6 @@ "commander": "^13.1.0", "open": "^10.2.0", "pino": "^9.14.0", - "undici": "^8.9.0", "zod": "^4.4.3" }, "devDependencies": { diff --git a/clients/cli/src/cli.ts b/clients/cli/src/cli.ts index f4d418ae..c1a3673d 100644 --- a/clients/cli/src/cli.ts +++ b/clients/cli/src/cli.ts @@ -17,6 +17,7 @@ import { clearStoredAuthForRelogin } from "./clear-stored-auth-for-relogin.js"; import { InspectorClient } from "@inspector/core/mcp/index.js"; import { cleanRoots } from "@inspector/core/mcp/serverList.js"; import { + createProxyFetch, createTransportNode, loadServerEntries, selectServerEntry, @@ -24,6 +25,8 @@ import { parseHeaderPair, } from "@inspector/core/mcp/node/index.js"; import type { JsonValue } from "@inspector/core/mcp/index.js"; +import type { StrictJsonValue } from "@inspector/core/json/jsonUtils.js"; +import { isSerializableJson } from "@inspector/core/json/jsonUtils.js"; import { canonicalUrlHost, isAllInterfacesHost, @@ -33,7 +36,6 @@ import { consumeMethodOutcome } from "./handlers/consume-outcome.js"; import { runMethod } from "./handlers/run-method.js"; import { isOneShotMethod, - metaValueToString, ONE_SHOT_METHODS, type MethodArgs, } from "./handlers/method-types.js"; @@ -122,6 +124,11 @@ async function callMethod( const environment: InspectorClientEnvironment = { transport: createTransportNode, + // Proxy support sits at the bottom of the fetch stack so InspectorClient's + // wrappers compose over it — and so OAuth discovery/token requests, which + // also run through `environment.fetch`, are proxied too (#2067). Undefined + // when no proxy env var is set, which leaves the built-in fetch in place. + fetch: createProxyFetch(), }; const redirectUrlProvider = new MutableRedirectUrlProvider(); // Disarmed until the CLI-owned interactive OAuth flow runs — SDK `auth()` @@ -162,6 +169,17 @@ async function callMethod( // the change at all: the SDK refuses `roots/list_changed` from a client // that never declared it, which `setRoots` logged as a send failure (#1797). roots: cleanRoots(serverSettings?.roots ?? []), + // Per-server default `_meta` from mcp.json, exactly as web (`App.tsx`) and + // the TUI pass it — the setting belongs to the server, not to the client + // that happens to read it, and `InspectorClient` only reads the option + // rather than falling back to `serverSettings.metadata` (#2093). Already a + // JSON object (#1910), so there is no pair-array flattening left to do; + // `{}` means "no defaults". `--metadata` stays per-invocation and wins on a + // key collision, since call-time keys override defaults in `mergeMeta`. + ...(serverSettings?.metadata && + Object.keys(serverSettings.metadata).length > 0 && { + defaultMetadata: serverSettings.metadata, + }), serverSettings, // Per-server protocol era (SEP §7.8) from mcp.json → SDK versionNegotiation. // Absent era defaults to legacy in the InspectorClient constructor (#1626). @@ -445,6 +463,11 @@ function buildHandoff( : canonicalUrlHost(host); const clientPort = process.env.CLIENT_PORT || "6274"; const sandboxPort = process.env.MCP_SANDBOX_PORT || "6275"; + // The dedicated app origin (#2056). Forwarded alongside the other two: an App + // whose UI resource declares `_meta.ui.domain` is served from this port and + // the browser reaches it DIRECTLY, so a handoff that forwards only 6274/6275 + // renders that app from an unreachable origin. + const appOriginPort = process.env.MCP_APP_ORIGIN_PORT || "6278"; // Treat an empty MCP_INSPECTOR_API_TOKEN the same as unset — an empty token // can't satisfy the deep-link autoConnect gate. const apiToken = process.env.MCP_INSPECTOR_API_TOKEN || undefined; @@ -462,7 +485,7 @@ function buildHandoff( return { serverUrl: normalizedUrl, deepLink: `http://${linkHost}:${clientPort}/?${params.toString()}`, - portForwardCmd: `coder port-forward --tcp ${clientPort}:${clientPort} --tcp ${sandboxPort}:${sandboxPort}`, + portForwardCmd: `coder port-forward --tcp ${clientPort}:${clientPort} --tcp ${sandboxPort}:${sandboxPort} --tcp ${appOriginPort}:${appOriginPort}`, oauthStatePath: statePath, apiToken: apiToken ?? null, note: @@ -474,8 +497,8 @@ function buildHandoff( function parseKeyValuePair( value: string, - previous: Record = {}, -): Record { + previous: Record = {}, +): Record { const parts = value.split("="); const key = parts[0]; const val = parts.slice(1).join("="); @@ -486,13 +509,31 @@ function parseKeyValuePair( ); } - let parsedValue: JsonValue; + // `StrictJsonValue`: `JSON.parse` cannot produce `undefined`, and these values + // become `_meta`, which must reach the wire exactly as written (#1910). + let parsedValue: StrictJsonValue; try { - parsedValue = JSON.parse(val) as JsonValue; + parsedValue = JSON.parse(val) as StrictJsonValue; } catch { + // Not JSON at all — a bare word or an unquoted string. Sent as a string, + // which is what the user plainly meant. parsedValue = val; } + // Valid JSON syntax is not the same as sendable JSON: `1e400` parses to + // `Infinity`, which `JSON.stringify` writes as `null`. Rejecting is better + // than accepting the flag and silently transmitting a different value — + // and better than falling back to the literal string, which would also not + // be what was asked for. + if (!isSerializableJson(parsedValue)) { + // Names the key, never the value: the pair can carry a credential + // (`credentials={"accessToken":"…","n":1e400}`) and this message lands in + // stderr and CI logs. + throw new Error( + `Invalid value for "${key}": numbers must be finite (a literal like 1e400 overflows to Infinity and cannot be sent).`, + ); + } + return { ...previous, [key as string]: parsedValue }; } @@ -736,8 +777,8 @@ async function parseArgs(argv?: string[]): Promise { promptName?: string; promptArgs?: Record; logLevel?: LoggingLevel; - metadata?: Record; - toolMetadata?: Record; + metadata?: Record; + toolMetadata?: Record; cwd?: string; transport?: "sse" | "http" | "stdio"; serverUrl?: string; @@ -999,22 +1040,12 @@ async function parseArgs(argv?: string[]): Promise { promptName: options.promptName, promptArgs: options.promptArgs, logLevel: options.logLevel, - metadata: options.metadata - ? Object.fromEntries( - Object.entries(options.metadata).map(([key, value]) => [ - key, - metaValueToString(value), - ]), - ) - : undefined, - toolMeta: options.toolMetadata - ? Object.fromEntries( - Object.entries(options.toolMetadata).map(([key, value]) => [ - key, - metaValueToString(value), - ]), - ) - : undefined, + // `--metadata`/`--tool-metadata` values are parsed as JSON, and `_meta` + // takes any JSON — so they go through unflattened (#1910). They used to be + // squeezed through `metaValueToString`, which sent `{"a":1}` as the + // *string* `'{"a":1}'`. + metadata: options.metadata, + toolMeta: options.toolMetadata, appInfo: options.appInfo === true, format: options.format, }; diff --git a/clients/cli/src/handlers/collect-app-info.ts b/clients/cli/src/handlers/collect-app-info.ts index aa2df1fc..d46e3754 100644 --- a/clients/cli/src/handlers/collect-app-info.ts +++ b/clients/cli/src/handlers/collect-app-info.ts @@ -2,6 +2,7 @@ import { InspectorClient } from "@inspector/core/mcp/index.js"; import { extractAppInfo } from "@inspector/core/mcp/apps.js"; import type { AppInfo } from "@inspector/core/mcp/apps.js"; import type { CliAppInfo } from "./method-types.js"; +import type { RequestMetadata } from "@inspector/core/mcp/types.js"; /** * Build the CLI's app-info for a tool. Never throws — failures fold into @@ -10,7 +11,7 @@ import type { CliAppInfo } from "./method-types.js"; export async function collectAppInfo( client: Pick, tool: Parameters[0], - metadata: Record | undefined, + metadata: RequestMetadata | undefined, ): Promise { let base: AppInfo; try { diff --git a/clients/cli/src/handlers/connect-timeout.ts b/clients/cli/src/handlers/connect-timeout.ts index 7d5a7b2c..8bd8cf65 100644 --- a/clients/cli/src/handlers/connect-timeout.ts +++ b/clients/cli/src/handlers/connect-timeout.ts @@ -22,7 +22,7 @@ export function withConnectTimeout( if (settings) return { ...settings, connectionTimeout }; return { headers: [], - metadata: [], + metadata: {}, env: [], connectionTimeout, requestTimeout: 0, diff --git a/clients/cli/src/handlers/method-types.ts b/clients/cli/src/handlers/method-types.ts index b0505425..424b941b 100644 --- a/clients/cli/src/handlers/method-types.ts +++ b/clients/cli/src/handlers/method-types.ts @@ -1,4 +1,5 @@ import type { JsonValue } from "@inspector/core/mcp/index.js"; +import type { RequestMetadata } from "@inspector/core/mcp/types.js"; import type { AppInfo } from "@inspector/core/mcp/apps.js"; import type { LoggingLevel } from "@modelcontextprotocol/client"; import type { OutputFormat } from "./format-output.js"; @@ -20,8 +21,8 @@ export type MethodArgs = { logLevel?: LoggingLevel; toolName?: string; toolArg?: Record; - toolMeta?: Record; - metadata?: Record; + toolMeta?: RequestMetadata; + metadata?: RequestMetadata; appInfo?: boolean; format?: OutputFormat; /** Task id for tasks/get, tasks/cancel, tasks/result. */ @@ -110,13 +111,3 @@ export type OneShotMethod = (typeof ONE_SHOT_METHODS)[number]; export function isOneShotMethod(method: string): method is OneShotMethod { return (ONE_SHOT_METHODS as readonly string[]).includes(method); } - -/** - * Encode a metadata / tool-metadata value for MethodArgs (string map). - * Strings pass through; objects/arrays use JSON.stringify so structure is kept - * (avoids String(obj) → "[object Object]"). - */ -export function metaValueToString(value: JsonValue): string { - if (typeof value === "string") return value; - return JSON.stringify(value); -} diff --git a/clients/cli/src/handlers/servers-list.ts b/clients/cli/src/handlers/servers-list.ts index 78bd896f..d580f47c 100644 --- a/clients/cli/src/handlers/servers-list.ts +++ b/clients/cli/src/handlers/servers-list.ts @@ -157,10 +157,10 @@ export function sanitizeServerSettings( key: h.key, value: isSensitiveHeader(h.key) ? REDACTED : h.value, })), - metadata: (settings.metadata ?? []).map((m) => ({ - key: m.key, - value: isSensitiveHeader(m.key) ? REDACTED : m.value, - })), + // Metadata values may be any JSON (#1910), so the redaction has to descend: + // a top-level key that is not itself sensitive can hold a nested one that + // is. See `redactMetadataValue`. + metadata: redactMetadataValue(settings.metadata ?? {}), env: (settings.env ?? []).map((e) => ({ key: e.key, value: REDACTED, @@ -212,6 +212,39 @@ function sanitizeInitRecord( return out; } +/** + * Redact secret-bearing entries anywhere inside a `_meta` payload. + * + * The pre-#1910 version only had to check the top level, because a value was + * always a string — a secret could not hide below one. Now that a value may be + * an object or an array, `{ trace: { accessToken: "…" } }` would sail through a + * top-level-only check: `trace` is not sensitive, so the whole subtree, + * `accessToken` included, would print. `servers/show` output is meant to be + * safe to paste into an issue, so the walk has to reach every key. + * + * A key matching {@link isSensitiveHeader} replaces its **entire** value rather + * than being descended into, so a sensitive key holding an object cannot leak + * through its members either. Arrays are mapped elementwise: their indices are + * not names anyone can mark sensitive, but objects inside them can be. + * + * Recursion is bounded by the payload, which came from `JSON.parse` and so is a + * finite tree with no cycles. + */ +function redactMetadataValue(value: unknown): unknown { + if (Array.isArray(value)) { + return value.map(redactMetadataValue); + } + if (value !== null && typeof value === "object") { + return Object.fromEntries( + Object.entries(value).map(([key, member]) => [ + key, + isSensitiveHeader(key) ? REDACTED : redactMetadataValue(member), + ]), + ); + } + return value; +} + function isSensitiveHeader(key: string): boolean { const k = key.toLowerCase(); return ( diff --git a/clients/cli/tsup.config.ts b/clients/cli/tsup.config.ts index c37897ca..74e70568 100644 --- a/clients/cli/tsup.config.ts +++ b/clients/cli/tsup.config.ts @@ -20,6 +20,14 @@ export default defineConfig({ // Bundle core source; leave npm deps external. noExternal: [/^@inspector\/core/], external: [ + // `undici` MUST stay external. It is CommonJS, so inlining it rewrites + // `import("undici")` to a relative chunk whose `require("assert")` hits + // esbuild's ESM `__require` shim and throws "Dynamic require of \"assert\" + // is not supported" — and because the specifier was rewritten at build time, + // no user-side install can ever satisfy it (#2067). It is declared in the + // ROOT manifest only, so tsup cannot infer this from a nearest-manifest + // lookup; the entry has to be explicit. + "undici", "@napi-rs/keyring", // Root-declared (see the repo's dependency-placement rule) and CJS, which // is the combination that bites: tsup externalizes what the *client's* @@ -30,6 +38,12 @@ export default defineConfig({ "proper-lockfile", "@modelcontextprotocol/client", "@modelcontextprotocol/core", + // Root-declared and reached through `core/mcp/apps.ts`, so it must be + // external here like every other root runtime dependency (AGENTS.md). This + // client was actually inlining it — dragging the transitive v1 SDK and + // zod-to-json-schema in with it — which the #2067 guard surfaced. ESM, so it + // was not failing the way `undici` did; the rule is what it violated. + "@modelcontextprotocol/ext-apps", "commander", "pino", ], diff --git a/clients/tui/__tests__/App.test.tsx b/clients/tui/__tests__/App.test.tsx index a66fe603..e58d8c77 100644 --- a/clients/tui/__tests__/App.test.tsx +++ b/clients/tui/__tests__/App.test.tsx @@ -102,7 +102,18 @@ const h = vi.hoisted(() => { // handler for the event (the common single-server case). type EventEntry = { client: unknown; fn: (event: unknown) => void }; const clientEvents = new Map>(); - const clientInstances: Array<{ cfg?: { type?: string; url?: string } }> = []; + const clientInstances: Array<{ + cfg?: { type?: string; url?: string }; + // The options the mount effect built for this server. Captured so a test + // can assert what was actually handed to the client — `defaultMetadata` + // and the `environment.fetch` proxy wiring (#2067), both of which are + // invisible to a render-only assertion. + opts?: { defaultMetadata?: unknown; environment?: { fetch?: unknown } }; + }> = []; + // What the mocked `createProxyFetch` returns. `undefined` is the real + // behavior with no proxy env var set; a test points this at a sentinel to + // exercise the proxy-configured branch (#2067). + const proxyFetchState: { current: unknown } = { current: undefined }; const fireClientEvent = (event: string, detail?: unknown) => { clientEvents.get(event)?.forEach((e) => e.fn({ detail })); }; @@ -127,8 +138,15 @@ const h = vi.hoisted(() => { } class FakeClient { cfg: { type?: string; url?: string } | undefined; - constructor(config?: { type?: string; url?: string }) { + opts: + | { defaultMetadata?: unknown; environment?: { fetch?: unknown } } + | undefined; + constructor( + config?: { type?: string; url?: string }, + opts?: { defaultMetadata?: unknown; environment?: { fetch?: unknown } }, + ) { this.cfg = config; + this.opts = opts; clientInstances.push(this); } // Derive the transport type from the server config the client was built @@ -183,6 +201,7 @@ const h = vi.hoisted(() => { } return { ctrl, + proxyFetchState, connect, disconnect, openUrl, @@ -232,6 +251,12 @@ vi.mock("@inspector/core/mcp/state/index.js", () => ({ })); vi.mock("@inspector/core/mcp/node/index.js", () => ({ createTransportNode: vi.fn(), + // Defaults to undefined — "no proxy configured", which is what the real one + // returns with no proxy env var set, so `environment.fetch` stays unset and + // the client falls back to the built-in fetch, exactly as before #2067. A + // test can point `h.proxyFetchState.current` at a sentinel to exercise the + // other branch. + createProxyFetch: vi.fn(() => h.proxyFetchState.current), })); vi.mock("@inspector/core/react/useInspectorClient.js", () => ({ useInspectorClient: h.useInspectorClient, @@ -340,7 +365,7 @@ function oneEmaHttp(): Record { config: { type: "streamable-http", url: "http://localhost:8080/mcp" }, settings: { requestTimeout: 0, - metadata: [], + metadata: {}, headers: [], env: [], roots: [], @@ -393,10 +418,10 @@ function httpWithSettings(): Record { config: { type: "streamable-http", url: "http://x" }, settings: { requestTimeout: 5000, - metadata: [ - { key: "team", value: "alpha" }, - { key: " ", value: "ignored" }, - ], + // Object-shaped with a nested value (#1910) — the shape the TUI now + // forwards verbatim. As a pair array this would have reached the wire + // as numeric `_meta` keys. + metadata: { team: "alpha", trace: { id: "abc", hops: [1, 2] } }, oauthClientId: "cid", oauthClientSecret: "secret", oauthScopes: "read write", @@ -633,6 +658,7 @@ beforeEach(() => { h.callbackStop.mockClear(); h.clientEvents.clear(); h.clientInstances.length = 0; + h.proxyFetchState.current = undefined; h.runner.override = null; h.clientSpies.authenticate.mockReset(); h.clientSpies.authenticate.mockResolvedValue( @@ -1114,6 +1140,13 @@ describe("App (input handling, focus, effects)", () => { it("builds a client with saved settings (metadata, oauth, timeout)", async () => { const r = await mount(httpWithSettings()); await expectFrame(r, "MCP Servers"); + // Not just "it mounted": the saved metadata must reach the client as the + // same object, nesting intact (#1910). + const web = h.clientInstances.find((c) => c.cfg?.url === "http://x"); + expect(web?.opts?.defaultMetadata).toEqual({ + team: "alpha", + trace: { id: "abc", hops: [1, 2] }, + }); }); it("passes top-level oauth client credentials into an http client", async () => { @@ -1608,6 +1641,27 @@ describe("App (OAuth result branches)", () => { await expectFrame(r, "Authorization updated. Retry your action"); }); + it("installs the proxy fetch as environment.fetch when a proxy is configured", async () => { + // The bottom-of-stack wiring for #2067. It is one line in App.tsx and it is + // what makes InspectorClient's own wrappers compose OVER the proxy instead + // of discarding it — delete it and TUI proxy support silently disappears + // while every other test stays green. + const sentinel: typeof fetch = async () => new Response(""); + h.proxyFetchState.current = sentinel; + await mount(oneHttp()); + expect(h.clientInstances.length).toBeGreaterThan(0); + expect(h.clientInstances[0]!.opts?.environment?.fetch).toBe(sentinel); + }); + + it("leaves environment.fetch unset when no proxy is configured", async () => { + // The default path must stay on the built-in fetch — a wrapper installed + // unconditionally would put every TUI user behind an undici fetch. + h.proxyFetchState.current = undefined; + await mount(oneHttp()); + expect(h.clientInstances.length).toBeGreaterThan(0); + expect(h.clientInstances[0]!.opts?.environment?.fetch).toBeUndefined(); + }); + it("ignores auth lifecycle events from a non-selected server", async () => { const r = await mount(twoHttp()); // web is selected; api is not await press(r, ["a"]); diff --git a/clients/tui/__tests__/helpers/oauth-test-fakes.ts b/clients/tui/__tests__/helpers/oauth-test-fakes.ts index efff339e..5e6e0fae 100644 --- a/clients/tui/__tests__/helpers/oauth-test-fakes.ts +++ b/clients/tui/__tests__/helpers/oauth-test-fakes.ts @@ -19,7 +19,7 @@ export function makeFakeServerSettings( ): InspectorServerSettings { return { headers: [], - metadata: [], + metadata: {}, env: [], connectionTimeout: 0, requestTimeout: 0, diff --git a/clients/tui/src/App.tsx b/clients/tui/src/App.tsx index 240bbd82..d8a8aee0 100644 --- a/clients/tui/src/App.tsx +++ b/clients/tui/src/App.tsx @@ -31,7 +31,10 @@ import { FetchRequestLogState, StderrLogState, } from "@inspector/core/mcp/state/index.js"; -import { createTransportNode } from "@inspector/core/mcp/node/index.js"; +import { + createProxyFetch, + createTransportNode, +} from "@inspector/core/mcp/node/index.js"; import { useInspectorClient } from "@inspector/core/react/useInspectorClient.js"; import { useManagedTools } from "@inspector/core/react/useManagedTools.js"; import { useManagedResources } from "@inspector/core/react/useManagedResources.js"; @@ -290,14 +293,14 @@ function App({ const environment: InspectorClientEnvironment = { transport: createTransportNode, logger: getTuiLogger(), + // Bottom of the fetch stack, so InspectorClient's wrappers compose + // over it and OAuth requests are proxied too (#2067). Undefined when + // no proxy env var is set. + fetch: createProxyFetch(), }; - const defaultMetadata = savedSettings?.metadata - ? Object.fromEntries( - savedSettings.metadata - .filter((m) => m.key.trim() !== "") - .map((m) => [m.key, m.value]), - ) - : undefined; + // Per-server default `_meta` is already a JSON object (#1910) — no + // pair-array flattening left to do; `{}` means "no defaults". + const defaultMetadata = savedSettings?.metadata; const clientAuthOptions = buildRunnerClientAuthOptions( clientConfig, savedSettings, diff --git a/clients/tui/tsup.config.ts b/clients/tui/tsup.config.ts index 109011ba..60fc9827 100644 --- a/clients/tui/tsup.config.ts +++ b/clients/tui/tsup.config.ts @@ -99,6 +99,14 @@ export default defineConfig({ // `__tests__/tsupConfig.test.ts` guards this list. noExternal: [/^@inspector\/core/, "ink-form", "ink-scroll-view"], external: [ + // `undici` MUST stay external. It is CommonJS, so inlining it rewrites + // `import("undici")` to a relative chunk whose `require("assert")` hits + // esbuild's ESM `__require` shim and throws "Dynamic require of \"assert\" + // is not supported" — and because the specifier was rewritten at build time, + // no user-side install can ever satisfy it (#2067). It is declared in the + // ROOT manifest only, so tsup cannot infer this from a nearest-manifest + // lookup; the entry has to be explicit. + "undici", // `react` is deliberately external — the single instance every inlined // package above resolves to, from this build directory. "react", @@ -119,6 +127,12 @@ export default defineConfig({ "pino", "@modelcontextprotocol/client", "@modelcontextprotocol/core", + // Root-declared and reached through `core/mcp/apps.ts`, so it must be + // external here like every other root runtime dependency — which client + // actually reaches it is a function of what `core/` imports, not of this + // client's own code, so all three lists carry it (AGENTS.md). The CLI was + // inlining it; the #2067 guard surfaced that. + "@modelcontextprotocol/ext-apps", "@napi-rs/keyring", // Root-declared (see the repo's dependency-placement rule) and CJS, which // is the combination that bites: tsup externalizes what the *client's* diff --git a/clients/web/.storybook/preview.tsx b/clients/web/.storybook/preview.tsx index 6b419a43..e1669ea0 100644 --- a/clients/web/.storybook/preview.tsx +++ b/clients/web/.storybook/preview.tsx @@ -1,14 +1,11 @@ import type { Preview } from "@storybook/react-vite"; -import { - MantineProvider, - useMantineColorScheme, - type CSSVariablesResolver, -} from "@mantine/core"; +import { MantineProvider, useMantineColorScheme } from "@mantine/core"; import { Notifications } from "@mantine/notifications"; import "@mantine/core/styles.css"; import "@mantine/notifications/styles.css"; import "../src/App.css"; import { theme } from "../src/theme/theme"; +import { cssVariablesResolver } from "../src/theme/cssVariables"; import { useEffect } from "react"; // eslint-disable-next-line react-refresh/only-export-components @@ -28,14 +25,6 @@ function ColorSchemeWrapper({ return <>{children}; } -const resolver: CSSVariablesResolver = () => ({ - variables: {}, - light: {}, - dark: { - "--mantine-color-body": "var(--mantine-color-dark-9)", - }, -}); - const preview: Preview = { globalTypes: { colorScheme: { @@ -61,7 +50,7 @@ const preview: Preview = { &transport=http|sse&autoConnect= ``` -| Param | Meaning | -| --- | --- | -| `serverUrl` | The MCP server URL. Restricted to `http:` / `https:` (a crafted `javascript:` / `data:` / `file:` value is rejected). Canonicalized via `URL.href` so it matches the OAuth store's key form. | -| `transport` | `http` (streamable-HTTP, the default) or `sse`. Unknown values fall back to `http`. | +| Param | Meaning | +| ------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `serverUrl` | The MCP server URL. Restricted to `http:` / `https:` (a crafted `javascript:` / `data:` / `file:` value is rejected). Canonicalized via `URL.href` so it matches the OAuth store's key form. | +| `transport` | `http` (streamable-HTTP, the default) or `sse`. Unknown values fall back to `http`. | | `autoConnect` | **CSRF gate.** Must equal the per-launch `MCP_INSPECTOR_API_TOKEN`. The token is random per launch and only known to whatever started the server, so a third-party-minted link cannot satisfy it — this is the same exposure surface as the existing `?MCP_INSPECTOR_API_TOKEN=` param. Without a match the link is ignored. | The deep link upserts a stable `deep-link` catalog row (so a reload reconnects to the same row instead of accumulating duplicates) and connects. Connection-level outcomes are surfaced on the `AppShell.Header` as a machine-readable contract, so a driver can `waitForSelector` and read the failure reason without scraping a transient toast: -| Attribute | Where | Meaning | -| --- | --- | --- | -| `data-testid="connection-status"` | header | The element carrying the attributes below. | -| `data-status` | on `connection-status` | The live `ConnectionStatus` (`disconnected` → `connecting` → `connected` / `error`). Poll for `connected`. | -| `data-error-message` | on `connection-status` | Why the last connect failed (handshake error, OAuth-start failure, deep-link automation failure); absent when there is no error. | -| `data-deeplink` | on `connection-status` | `parsed` (a valid deep link drove this load), `rejected` (deep-link params present but the token/serverUrl gate failed), or `none`. Lets a driver distinguish "no deep link" from "rejected" — both otherwise leave `data-status` idle. | +| Attribute | Where | Meaning | +| --------------------------------- | ---------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `data-testid="connection-status"` | header | The element carrying the attributes below. | +| `data-status` | on `connection-status` | The live `ConnectionStatus` (`disconnected` → `connecting` → `connected` / `error`). Poll for `connected`. | +| `data-error-message` | on `connection-status` | Why the last connect failed (handshake error, OAuth-start failure, deep-link automation failure); absent when there is no error. | +| `data-deeplink` | on `connection-status` | `parsed` (a valid deep link drove this load), `rejected` (deep-link params present but the token/serverUrl gate failed), or `none`. Lets a driver distinguish "no deep link" from "rejected" — both otherwise leave `data-status` idle. | ### Landing on a rendered app @@ -127,27 +127,132 @@ Three further params extend the deep link to pre-select — and optionally auto- …&openApp=&appArgs=&autoOpen= ``` -| Param | Meaning | -| --- | --- | -| `openApp` | The app-tool name. Once the connection is up and the tool appears in the app list, the inspector switches to the Apps tab and pre-selects it. | -| `appArgs` | `base64url(JSON)` object of form values. Merged **over** the tool's schema defaults (`collectSchemaDefaults`) so a required-with-default field isn't left blank — which would otherwise disable "Open App". Malformed / non-object values fall back to `{}`. | -| `autoOpen` | **Same CSRF gate as `autoConnect`** — must equal the session token. When set, "Open App" fires automatically (a tool call from a URL), so the token gate is mandatory. Without a match the app is pre-selected but not opened. | +| Param | Meaning | +| ---------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| `openApp` | The app-tool name. Once the connection is up and the tool appears in the app list, the inspector switches to the Apps tab and pre-selects it. | +| `appArgs` | `base64url(JSON)` object of form values. Merged **over** the tool's schema defaults (`collectSchemaDefaults`) so a required-with-default field isn't left blank — which would otherwise disable "Open App". Malformed / non-object values fall back to `{}`. | +| `autoOpen` | **Same CSRF gate as `autoConnect`** — must equal the session token. When set, "Open App" fires automatically (a tool call from a URL), so the token gate is mandatory. Without a match the app is pre-selected but not opened. | The app-side render lifecycle is observable through the [MCP Apps screen automation contract](#mcp-apps-screen-automation-contract) above (`data-app-status="ready"`), so a driver can `waitForSelector` the whole `connect → open → ready` chain deterministically. +## MCP App dedicated origins (`_meta.ui.domain`) + +By default an MCP App is rendered by handing its HTML to the sandbox proxy as +`srcdoc`, inside an iframe sandboxed **without** `allow-same-origin`. That is +the isolation model from #1565 and it stays the default — but it gives the app +document an *opaque* origin, so every request it makes carries `Origin: null`. +An app whose backend allowlists origins (CORS, an OAuth callback, an API-key +allowlist) can't work that way, which is what the spec's `_meta.ui.domain` +exists to solve: a server uses it to ask its host for a stable, dedicated +origin. + +**The Inspector's host-specific contract.** The spec makes `domain`'s format +host-dependent ("servers MUST consult host-specific documentation"), and the +Inspector owns no domain infrastructure — it cannot serve +`my-app.example.com`. So it treats the field as a **request, not an address**: + +- Any **non-empty** `domain` string opts the resource in. The value itself is + not parsed, matched, or reserved — declare whatever your production host + expects. +- The Inspector answers with a real HTTP origin on loopback: + `http://:`, default **`6278`** — in the same + `627x` family as the web port `6274` and the sandbox port `6275`, so the + three forward together. It is **not** `6276`: that is the fixed loopback + OAuth callback the CLI and TUI listen on, which OAuth apps pre-register and + which therefore cannot move. `6277` is skipped as v1's retired proxy port. +- The app document is served from there under an unguessable path, its + per-app CSP delivered as a real response **header** (stronger than the + `` the `srcdoc` path relies on) plus a `frame-ancestors` that admits + only the two origins in its real ancestor chain — the sandbox proxy that + frames it, **and** the Inspector page that frames the proxy. Both are + required, not belt-and-braces: `frame-ancestors` is checked against every + ancestor, so omitting the Inspector's own origin blocks the frame outright + (see `appDocumentEmbedders`). +- The inner iframe is granted `allow-same-origin` on this path **only** — + that is what makes the origin real rather than opaque. It is not a + weakening of #1565: the listener is on its own port, so the app is + cross-origin to both the sandbox proxy and the Inspector, and same-origin + policy blocks the reach either way. The proxy refuses the grant outright if + the URL's origin equals its own, and the grant is never reachable from the + server-supplied `sandbox` string, which is still stripped. + +**It is one shared origin, not one per app.** Every domain-declaring app is +served from the same port, keyed by path. That delivers the property the field +is *for* — a real, allowlistable origin — without minting a port or a DNS name +per app, at the cost of not being a per-app isolation boundary: two such apps +share `localStorage`, `sessionStorage`, and cookies for that origin. + +**And the separate port does not isolate cookies from the rest of loopback.** +A distinct port makes a distinct *origin*, so the origin-scoped surfaces — +DOM/scripting access, `localStorage`, `sessionStorage`, IndexedDB — are isolated +from the sandbox proxy and from the Inspector page. **Cookies are not +origin-scoped**: they are keyed by host and path, ignoring port. An app served +here therefore shares the `127.0.0.1` cookie jar with the Inspector on `6274` +and with anything else on loopback — it can read any non-`HttpOnly` cookie set +for that host, and set `Path=/` cookies those services will receive. + +This is not a hole in the Inspector's own auth: the API token travels in the +`x-mcp-remote-auth` header and lives in a `window` global and `sessionStorage`, +neither of which another origin can read. Closing the cookie gap properly needs +a distinct *host*, which this listener cannot mint (the Inspector owns no DNS, +and a second loopback address is not portable) — so it is stated rather than +papered over. Don't run the Inspector beside a loopback service whose session +cookie you would not hand to an app under test. + +**Every failure falls back rather than blanking the app.** No app-origin +listener, a port that never bound, an older backend with no +`POST /api/app-document` route, a network error — each renders the app the +default (opaque-origin) way and logs a console warning naming `_meta.ui.domain`. +Losing the real origin degrades what the app can reach; losing the app itself +would be worse. + +**One failure is outside that guarantee, deliberately: a published document the +browser cannot reach.** Every case above is one the *host* observes — publishing +returned nothing, so it falls back before choosing a render path. If publishing +succeeds and the browser then cannot load the URL (the port is not forwarded, or +a collision moved the listener to a dynamic port that is not), the frame stays +blank instead. There is no honest signal to fall back on: the navigation is +cross-origin, so `onerror` never fires for an HTTP error, `onload` fires for the +error page too, and nothing about the document is readable. The alternatives are +a timeout — which races an app that is merely slow, and "recovers" it by +re-running it at an opaque origin, executing its side effects twice — or a +reachability probe on every render, which proves the port answers rather than +that this fetch will. So the cause is removed instead of guessed at: every +documented remote workflow forwards the port (see below, the Docker section of +the root README, and the SSH recipe in `docs/mcp-app-review.md`). + +**Forward `6278` too** if you need this off loopback (a container, a tunnel). +As with the sandbox port, a taken port falls back to an OS-assigned one with a +loud warning — the app still renders, but the origin an app's backend was told +to allowlist is then wrong, which is what the warning tells you. + ## Theme (`src/theme/`) Each customized Mantine component has a `Theme.ts` file (`Button.ts`, `Text.ts`, …, ~21 total) exporting a `Theme` constant; the barrel `index.ts` re-exports them and `theme.ts` assembles the `MantineProvider` theme. Theme files hold app-wide defaults and **variants** (flat CSS-in-JS); only pseudo-selectors, nested child selectors, keyframes, and native-HTML styling belong in `App.css`. Element components import from `@mantine/core` (never from `theme/`) — the theme layer is applied transparently by the provider. +**`cssVariables.ts` is the third piece, beside the component files and `App.css`.** It holds overrides for the CSS variables `MantineProvider` injects at runtime, which `App.css` cannot reach: the provider appends its generated `