diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index e6a22c98..63290884 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,7 +95,7 @@ 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. @@ -104,11 +104,11 @@ Both exist and do different jobs. Theme files (`src/theme/.ts`) custo - `npm run format` before committing; **`npm run ci` before pushing** (`validate` → `coverage` → `verify:build-gate` → `smoke` → Storybook). `npm run validate` is the fast inner-loop check and is **not** a substitute — it runs `test`, not `test:coverage`, so it does zero coverage gating. - **A dependency bump must land in every install that declares it.** v2 is not a workspace — the root and each `clients/*` have their own `node_modules`, and a client's test project compiles `core/` and `test-servers/src` (which resolve from the **root**) alongside the client's own sources. Bumping a shared dependency in one manifest only puts two versions of it in one `tsc` program; for a recursive-generic surface like zod that exhausts the tsc heap (#1896). `verify:dep-lockstep` fails the build on this — 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*. +- **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/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 67cd1596..04f870d9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -251,6 +251,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 derives what to check from each client's own `external` array, so a newly externalized package is covered without editing the guard. Two 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. @@ -795,7 +802,8 @@ The ⚠️ option-deletion hazard, the snapshot rule, and the recovery recipe ab - **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. + - **`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 `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. diff --git a/README.md b/README.md index 49528454..d1afcbab 100644 --- a/README.md +++ b/README.md @@ -367,6 +367,7 @@ 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 client declares `external` was inlined anyway (an emitted `-HASH.js` chunk, or a rewritten `./-HASH.js` specifier in the entry). `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 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`. | diff --git a/clients/cli/README.md b/clients/cli/README.md index d5c033df..d9f6c757 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 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..8542d768 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, @@ -122,6 +123,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()` diff --git a/clients/cli/tsup.config.ts b/clients/cli/tsup.config.ts index c37897ca..5518a7f1 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* diff --git a/clients/tui/__tests__/App.test.tsx b/clients/tui/__tests__/App.test.tsx index a66fe603..56ac7682 100644 --- a/clients/tui/__tests__/App.test.tsx +++ b/clients/tui/__tests__/App.test.tsx @@ -232,6 +232,10 @@ vi.mock("@inspector/core/mcp/state/index.js", () => ({ })); vi.mock("@inspector/core/mcp/node/index.js", () => ({ createTransportNode: vi.fn(), + // Returns undefined for "no proxy configured", which is what the real one + // does 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. + createProxyFetch: vi.fn(() => undefined), })); vi.mock("@inspector/core/react/useInspectorClient.js", () => ({ useInspectorClient: h.useInspectorClient, diff --git a/clients/tui/src/App.tsx b/clients/tui/src/App.tsx index 240bbd82..56ef0134 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,6 +293,10 @@ 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( diff --git a/clients/tui/tsup.config.ts b/clients/tui/tsup.config.ts index 109011ba..059ecc6a 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", diff --git a/clients/web/README.md b/clients/web/README.md index 19f77385..7b4ea4ee 100644 --- a/clients/web/README.md +++ b/clients/web/README.md @@ -193,4 +193,6 @@ In every case, exposing the Inspector beyond loopback also means anyone who can ## HTTP proxy support -The web backend connects to remote MCP servers through the shared Node transport (`core/mcp/node/transport.ts`), which honors the conventional proxy environment variables: `HTTPS_PROXY` / `HTTP_PROXY` (and their lowercase forms) select the proxy, and `NO_PROXY` exempts hosts. Routing is powered by [`undici`](https://www.npmjs.com/package/undici)'s `EnvHttpProxyAgent`, imported lazily only when a proxy variable is set, so runs without a proxy configured pay no cost. See the CLI README for more detail. +The web backend connects to remote MCP servers through the shared Node fetch (`core/mcp/node/proxyFetch.ts`), which honors the conventional proxy environment variables: `HTTPS_PROXY` / `HTTP_PROXY` (and their lowercase forms) select the proxy, and `NO_PROXY` exempts hosts. Routing uses [`undici`](https://www.npmjs.com/package/undici)'s own `fetch` bound to its `EnvHttpProxyAgent`, imported lazily only when a proxy variable is set, so runs without a proxy configured pay no cost. + +The proxy sits at the **bottom** of the fetch stack rather than wrapping the transport's fetch, and both undici halves must come from the same copy — see [HTTP proxy support](../cli/README.md#http-proxy-support) in the CLI README for why ([#2067](https://github.com/modelcontextprotocol/inspector/issues/2067)). diff --git a/clients/web/src/test/integration/mcp/node/transport.test.ts b/clients/web/src/test/integration/mcp/node/transport.test.ts index af68081f..d8615cab 100644 --- a/clients/web/src/test/integration/mcp/node/transport.test.ts +++ b/clients/web/src/test/integration/mcp/node/transport.test.ts @@ -1,10 +1,18 @@ -import { afterEach, describe, it, expect, vi } from "vitest"; -import { getServerType } from "@inspector/core/mcp/config.js"; import { - createTransportNode, + afterAll, + afterEach, + beforeAll, + describe, + it, + expect, + vi, +} from "vitest"; +import { getServerType } from "@inspector/core/mcp/config.js"; +import { createTransportNode } from "@inspector/core/mcp/node/transport.js"; +import { + createProxyFetch, readProxyEnv, - withProxyDispatcher, -} from "@inspector/core/mcp/node/transport.js"; +} from "@inspector/core/mcp/node/proxyFetch.js"; import type { InspectorServerSettings, MCPServerConfig, @@ -16,6 +24,8 @@ import { createTestServerInfo, } from "@modelcontextprotocol/inspector-test-server"; import { Client } from "@modelcontextprotocol/client"; +import { createServer, request, type Server } from "node:http"; +import type { AddressInfo } from "node:net"; describe("Transport", () => { describe("getServerType", () => { @@ -396,50 +406,27 @@ describe("Transport", () => { expect(readProxyEnv()).toBeUndefined(); }); - it("withProxyDispatcher returns the original fetch unchanged when no proxy is configured", () => { + it("createProxyFetch returns undefined when no proxy is configured", () => { clearProxyEnv(); - const base = vi.fn(); - expect(withProxyDispatcher(base as unknown as typeof fetch)).toBe(base); + expect(createProxyFetch()).toBeUndefined(); }); - it("withProxyDispatcher injects an EnvHttpProxyAgent dispatcher into RequestInit", async () => { + it("createProxyFetch returns a fetch when a proxy is configured", () => { clearProxyEnv(); process.env.HTTPS_PROXY = "http://proxy.example:3128"; - const calls: Array<{ - input: Parameters[0]; - init: RequestInit | undefined; - }> = []; - const base = vi.fn( - async (input: Parameters[0], init?: RequestInit) => { - calls.push({ input, init }); - return new Response("ok"); - }, - ); - const proxied = withProxyDispatcher(base as unknown as typeof fetch); - expect(proxied).not.toBe(base); - await proxied("https://example.com/mcp", { method: "POST" }); - expect(calls).toHaveLength(1); - expect(calls[0].input).toBe("https://example.com/mcp"); - expect(calls[0].init?.method).toBe("POST"); - const dispatcher = (calls[0].init as { dispatcher?: object }).dispatcher; - expect(dispatcher).toBeDefined(); - expect(dispatcher?.constructor.name).toBe("EnvHttpProxyAgent"); - // Second call reuses the same dispatcher (lazy singleton). - await proxied("https://example.com/mcp"); - const dispatcher2 = (calls[1].init as { dispatcher?: object }).dispatcher; - expect(dispatcher2).toBe(dispatcher); + expect(typeof createProxyFetch()).toBe("function"); }); - it("createTransportNode wraps the supplied fetch with the proxy dispatcher for streamable-http", async () => { + it("createTransportNode leaves a supplied fetchFn untouched under a proxy", async () => { + // The proxy now lives at the *bottom* of the stack (environment.fetch), so + // a caller-supplied fetch must be used verbatim — the old behavior wrapped + // it and injected a `dispatcher`, which is what #2067 removed. clearProxyEnv(); process.env.HTTPS_PROXY = "http://proxy.example:3128"; - let seenDispatcher: object | undefined; + let seenInit: RequestInit | undefined; const fetchFn = vi.fn( async (_input: Parameters[0], init?: RequestInit) => { - seenDispatcher = (init as { dispatcher?: object } | undefined) - ?.dispatcher; - // Reject after capturing so connect() fails fast without hitting a - // real proxy. + seenInit = init; throw new Error("stop"); }, ); @@ -450,24 +437,173 @@ describe("Transport", () => { const client = new Client({ name: "t", version: "1" }); await expect(client.connect(result.transport)).rejects.toThrow(); expect(fetchFn).toHaveBeenCalled(); - expect(seenDispatcher?.constructor.name).toBe("EnvHttpProxyAgent"); + expect( + (seenInit as { dispatcher?: unknown } | undefined)?.dispatcher, + ).toBeUndefined(); }); - it("throws an actionable error when a proxy is configured but undici cannot be loaded", async () => { + describe("through a real proxy server", () => { + // A forwarding proxy, so these exercise the actual undici code path rather + // than a mock. That matters: #2067's second bug was a *runtime* interface + // mismatch between userland undici's dispatcher and Node's built-in fetch + // ("invalid onRequestStart method"), which every mock-based test missed + // because no mock ever dispatches. Only a request that really leaves the + // process can catch that class. + let proxy: Server; + let proxyUrl: string; + let origin: Server; + let originUrl: string; + const proxied: string[] = []; + const seenRequests: Array<{ method: string; probe: string }> = []; + + beforeAll(async () => { + origin = createServer((req, res) => { + seenRequests.push({ + method: req.method ?? "", + probe: String(req.headers["x-probe"] ?? ""), + }); + if (req.url === "/no-content") { + res.writeHead(204); + res.end(); + return; + } + res.writeHead(200, { "content-type": "text/plain", "x-origin": "1" }); + res.end("hello from origin"); + }); + await new Promise((r) => origin.listen(0, "127.0.0.1", r)); + originUrl = `http://127.0.0.1:${(origin.address() as AddressInfo).port}`; + + proxy = createServer((req, res) => { + // Absolute-form request URI is what makes this a proxy request. + proxied.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); + }); + await new Promise((r) => proxy.listen(0, "127.0.0.1", r)); + proxyUrl = `http://127.0.0.1:${(proxy.address() as AddressInfo).port}`; + }); + + afterAll(async () => { + await new Promise((r) => proxy.close(() => r())); + await new Promise((r) => origin.close(() => r())); + }); + + it("routes requests through the proxy and returns a global Response", async () => { + clearProxyEnv(); + process.env.HTTP_PROXY = proxyUrl; + const proxyFetch = createProxyFetch(); + expect(proxyFetch).toBeDefined(); + + const before = proxied.length; + const res = await proxyFetch!(`${originUrl}/hello`); + + // The request really went through the proxy... + expect(proxied.length).toBe(before + 1); + expect(proxied[proxied.length - 1]).toBe(`${originUrl}/hello`); + + // ...and came back as a *global* Response, not undici's own class. The + // SDK branches on `input instanceof Response` when formatting OAuth + // errors, so a foreign Response silently degrades those messages. + expect(res).toBeInstanceOf(Response); + expect(res.headers).toBeInstanceOf(Headers); + expect(res.status).toBe(200); + expect(res.headers.get("x-origin")).toBe("1"); + expect(await res.text()).toBe("hello from origin"); + }); + + it("honors NO_PROXY", async () => { + clearProxyEnv(); + process.env.HTTP_PROXY = proxyUrl; + process.env.NO_PROXY = "127.0.0.1"; + const proxyFetch = createProxyFetch(); + + const before = proxied.length; + const res = await proxyFetch!(`${originUrl}/direct`); + + expect(res.status).toBe(200); + // Went straight to the origin — the proxy never saw it. + expect(proxied.length).toBe(before); + }); + + it("preserves a null-body status", async () => { + clearProxyEnv(); + process.env.HTTP_PROXY = proxyUrl; + const proxyFetch = createProxyFetch(); + + const res = await proxyFetch!(`${originUrl}/no-content`); + expect(res.status).toBe(204); + expect(res.body).toBeNull(); + }); + + it("accepts a URL instance and forwards the init", async () => { + clearProxyEnv(); + process.env.HTTP_PROXY = proxyUrl; + const proxyFetch = createProxyFetch(); + + const res = await proxyFetch!(new URL(`${originUrl}/from-url`), { + method: "POST", + headers: { "x-probe": "yes" }, + }); + expect(res.status).toBe(200); + expect(proxied[proxied.length - 1]).toBe(`${originUrl}/from-url`); + expect(seenRequests[seenRequests.length - 1]).toMatchObject({ + method: "POST", + probe: "yes", + }); + }); + + it("accepts a Request as input", async () => { + clearProxyEnv(); + process.env.HTTP_PROXY = proxyUrl; + const proxyFetch = createProxyFetch(); + + const res = await proxyFetch!( + new Request(`${originUrl}/from-request`, { method: "GET" }), + ); + expect(res.status).toBe(200); + expect(proxied[proxied.length - 1]).toBe(`${originUrl}/from-request`); + }); + }); + + it("reports a packaging fault when undici cannot be loaded", async () => { + // The message deliberately does NOT tell the user to install undici. It + // is a root dependency and always present; the only way this branch is + // reached in a published build is that the bundler mangled the specifier + // — which is exactly what #2067 was, and the old copy sent every affected + // user chasing an install that could not have helped. clearProxyEnv(); process.env.HTTPS_PROXY = "http://proxy.example:3128"; vi.doMock("undici", () => { throw new Error("Cannot find module 'undici'"); }); - // Re-import after the mock so the dynamic import("undici") inside - // withProxyDispatcher is intercepted. - const { withProxyDispatcher: wrap } = - await import("@inspector/core/mcp/node/transport.js"); - const proxied = wrap(globalThis.fetch); - await expect(proxied("https://example.com")).rejects.toThrow( - /HTTPS_PROXY.*undici.*not.*available/i, + // Re-import after the mock so the dynamic import() inside the module is + // intercepted — and so the module-level agent memo starts out empty. + vi.resetModules(); + const { createProxyFetch: create } = + await import("@inspector/core/mcp/node/proxyFetch.js"); + const proxied = create(); + await expect(proxied!("https://example.com")).rejects.toThrow( + /undici.*could not be loaded/i, ); vi.doUnmock("undici"); + vi.resetModules(); }); }); }); diff --git a/clients/web/tsup.runner.config.ts b/clients/web/tsup.runner.config.ts index a6512a48..ae36e831 100644 --- a/clients/web/tsup.runner.config.ts +++ b/clients/web/tsup.runner.config.ts @@ -17,6 +17,14 @@ export default defineConfig({ platform: "node", 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", "commander", "open", "pino", diff --git a/core/mcp/node/index.ts b/core/mcp/node/index.ts index a398f1c8..c3e657f4 100644 --- a/core/mcp/node/index.ts +++ b/core/mcp/node/index.ts @@ -21,3 +21,4 @@ export { } from "./servers.js"; export { rehydrateMcpConfigFromKeychain } from "./server-secrets.js"; export { createTransportNode } from "./transport.js"; +export { createProxyFetch, readProxyEnv } from "./proxyFetch.js"; diff --git a/core/mcp/node/proxyFetch.ts b/core/mcp/node/proxyFetch.ts new file mode 100644 index 00000000..b19265be --- /dev/null +++ b/core/mcp/node/proxyFetch.ts @@ -0,0 +1,192 @@ +import type { + Dispatcher, + RequestInit as UndiciRequestInit, + Response as UndiciResponse, +} from "undici"; + +/** + * The undici and DOM declarations of `RequestInit` / `BodyInit` describe the + * same wire contract; they differ only in *which realm's* `ReadableStream`, + * `Blob`, and `Headers` types they name, and TypeScript cannot relate two `lib` + * sets' structurally-identical stream types (their `pipeThrough` signatures + * differ in variance alone). At runtime there is one implementation — undici + * consumes exactly the values Node's own `fetch` does — so these two casts + * bridge a declaration gap rather than a real type difference, and they are kept + * to this one module boundary instead of leaking into callers. + */ +function asUndiciInit(init: RequestInit | undefined): UndiciRequestInit { + return init as UndiciRequestInit; +} + +/** See {@link asUndiciInit} — same declaration gap, opposite direction. */ +function asBodyInit(body: UndiciResponse["body"]): BodyInit | null { + return body as BodyInit | null; +} + +/** Standard proxy env vars, in the precedence undici's EnvHttpProxyAgent uses. */ +const PROXY_ENV_VARS = [ + "HTTPS_PROXY", + "https_proxy", + "HTTP_PROXY", + "http_proxy", +] as const; + +/** + * First proxy URL found in the environment, or `undefined` if none is set. + * Exported for tests and so callers can decide whether proxying is in effect. + */ +export function readProxyEnv(): string | undefined { + for (const name of PROXY_ENV_VARS) { + const value = process.env[name]; + if (value && value.trim() !== "") return value; + } + return undefined; +} + +/** + * Statuses for which the `Response` constructor rejects a non-null body. + * + * undici already returns a null body for these, so the guard is belt-and-braces + * — but a `Response` constructor throwing inside the adapter would surface as an + * opaque failure of the request itself, which is a bad trade for one `Set`. + */ +const NULL_BODY_STATUSES: ReadonlySet = new Set([ + 101, 103, 204, 205, 304, +]); + +/** + * Re-wraps an undici `Response` as a *global* one, preserving streaming. + * + * This is not ceremony. Userland undici's `Response` is a different class from + * `globalThis.Response`, so `res instanceof Response` is `false` for it — and the + * SDK branches on exactly that: `parseErrorResponse` (in + * `@modelcontextprotocol/client`) reads `input instanceof Response ? await + * input.text() : input`, so an undici response falls into the string branch and + * every OAuth error degrades to `Raw body: [object Response]`. Handing back a + * genuine global `Response` keeps every consumer on the class it tests for. + * + * The body passes through as a `ReadableStream` rather than being buffered, so + * SSE responses (the streamable-HTTP transport's whole event channel) still + * stream. `url` and `redirected` are not carried over — the `Response` + * constructor cannot set them — which is safe here only because nothing in this + * repo or the SDK reads either. + */ +function toGlobalResponse(res: UndiciResponse): Response { + const headers = new Headers(); + for (const [key, value] of res.headers) headers.append(key, value); + return new Response( + NULL_BODY_STATUSES.has(res.status) ? null : asBodyInit(res.body), + { + status: res.status, + statusText: res.statusText, + headers, + }, + ); +} + +/** + * Loads userland undici and binds its `fetch` to an `EnvHttpProxyAgent`. + * + * Both halves of that pair must come from the *same* undici. Injecting a + * userland dispatcher into Node's built-in `fetch` — what this module used to do + * — couples the two undici copies at the dispatcher handler interface, which is + * not stable across majors: Node 22 ships undici 6, Node 24 ships 7, Node 26 + * ships 8, so a userland undici 8 agent handed a built-in undici 7 handler is + * rejected with `invalid onRequestStart method` and the request never leaves the + * process (#2067). Using undici's own `fetch` keeps the handler on both sides of + * the call inside one copy, which is why it works unchanged from the Node 22.19 + * engine floor through Node 26. + */ +/** + * Process-wide, so every client shares one `EnvHttpProxyAgent` and therefore one + * connection pool. The memo is deliberately not per-`createProxyFetch()` call: + * the TUI builds an environment per configured server, and a pool each would + * multiply sockets against the proxy for no benefit. + */ +let loadedProxiedFetch: Promise | undefined; + +async function loadProxiedFetch(): Promise { + let undici: typeof import("undici"); + try { + undici = await import("undici"); + } catch (cause) { + throw new Error( + "HTTPS_PROXY / HTTP_PROXY is set but the `undici` package could not be " + + "loaded, so requests cannot be routed through the proxy. This is a " + + "packaging fault rather than something to fix by installing undici; " + + "please report it, or unset the proxy env var to fall back to a direct " + + "connection.", + { cause }, + ); + } + + const dispatcher: Dispatcher = new undici.EnvHttpProxyAgent(); + return async (input, init) => { + const [target, requestInit] = toUndiciRequest(input, init); + return toGlobalResponse( + await undici.fetch(target, { ...asUndiciInit(requestInit), dispatcher }), + ); + }; +} + +/** + * Normalizes fetch's `input`/`init` pair into something undici's `fetch` accepts. + * + * undici builds its own `Request` from `input`, and its constructor cannot + * consume a *global* `Request` — it would stringify it to `"[object Request]"` + * and request that as a URL. Every call site in this repo and in the SDK passes + * a string or `URL` today, so the unpacking branch is defensive; without it a + * future caller passing a `Request` would fail in a way that looks like a + * network error rather than a type mismatch. + */ +function toUndiciRequest( + input: RequestInfo | URL, + init: RequestInit | undefined, +): [string, RequestInit] { + if (input instanceof Request) { + return [ + input.url, + { + method: input.method, + headers: input.headers, + // A stream body needs `duplex: "half"`; the field is undici's, not in + // the DOM `RequestInit`, so it rides along as an extra property. + ...(input.body ? { body: input.body, duplex: "half" } : {}), + signal: input.signal, + ...init, + }, + ]; + } + return [input instanceof URL ? input.href : input, init ?? {}]; +} + +/** + * A fetch that routes through `HTTPS_PROXY` / `HTTP_PROXY`, honoring `NO_PROXY`, + * or `undefined` when no proxy env var is set. + * + * Returning `undefined` rather than an identity wrapper is what keeps non-proxy + * users paying nothing: callers assign the result straight into an optional + * `fetch` slot, so undici is never imported and the built-in `fetch` is used as + * before. + * + * The env is read once, when this is called; undici's `EnvHttpProxyAgent` then + * owns per-request `NO_PROXY` matching. The agent itself is created lazily on + * the first request so that merely constructing a client costs nothing. + * + * This belongs at the *bottom* of a client's fetch stack — the Node clients' + * `environment.fetch` — because proxying now means substituting the fetch rather + * than decorating one. Wrapping it around a supplied `fetchFn` (as the old + * `withProxyDispatcher` did inside `createTransportNode`) would discard that + * function's behavior; installing it underneath instead lets `InspectorClient`'s + * own wrappers — request tracking, OAuth endpoint overrides — compose over it. + * A side benefit: OAuth discovery and token requests go through + * `environment.fetch` too, and were never proxied before. + */ +export function createProxyFetch(): typeof fetch | undefined { + if (readProxyEnv() === undefined) return undefined; + + return async (input, init) => { + loadedProxiedFetch ??= loadProxiedFetch(); + return (await loadedProxiedFetch)(input, init); + }; +} diff --git a/core/mcp/node/transport.ts b/core/mcp/node/transport.ts index ba943da3..28f3a8cd 100644 --- a/core/mcp/node/transport.ts +++ b/core/mcp/node/transport.ts @@ -13,68 +13,7 @@ import { SSEClientTransport } from "@modelcontextprotocol/client"; import { StreamableHTTPClientTransport } from "@modelcontextprotocol/client"; import { createFetchTracker } from "../fetchTracking.js"; import { createAuthChallengeInterceptFetch } from "./authChallengeFetch.js"; -import type { Dispatcher } from "undici"; - -/** Node's `RequestInit` plus the undici-specific `dispatcher` option. */ -type NodeRequestInit = RequestInit & { dispatcher?: Dispatcher }; - -/** Standard proxy env vars, in the precedence undici's EnvHttpProxyAgent uses. */ -const PROXY_ENV_VARS = [ - "HTTPS_PROXY", - "https_proxy", - "HTTP_PROXY", - "http_proxy", -] as const; - -/** - * First proxy URL found in the environment, or `undefined` if none is set. - * Exported for tests and so callers can decide whether proxying is in effect. - */ -export function readProxyEnv(): string | undefined { - for (const name of PROXY_ENV_VARS) { - const value = process.env[name]; - if (value && value.trim() !== "") return value; - } - return undefined; -} - -/** - * Wraps a fetch function so outbound HTTP/HTTPS requests honor the standard - * `HTTPS_PROXY` / `HTTP_PROXY` / `NO_PROXY` environment variables. - * - * Node's built-in `fetch` (the bundled undici) accepts a `dispatcher` option - * in `RequestInit`; undici's `EnvHttpProxyAgent` reads the proxy env vars and - * routes per-request, so a single dispatcher covers both schemes and respects - * `NO_PROXY`. The `undici` package is loaded lazily on first call so callers - * that never set a proxy env var (TUI, web backend) need no extra dependency. - * If the proxy is configured but `undici` is unavailable, the wrapper throws - * with an actionable message rather than silently ignoring the env var. - */ -export function withProxyDispatcher(baseFetch: typeof fetch): typeof fetch { - if (readProxyEnv() === undefined) return baseFetch; - - let dispatcherPromise: Promise | undefined; - const getDispatcher = (): Promise => { - dispatcherPromise ??= import("undici").then( - ({ EnvHttpProxyAgent }) => new EnvHttpProxyAgent(), - (cause: unknown) => { - throw new Error( - "HTTPS_PROXY / HTTP_PROXY is set but the `undici` package is not " + - "available. Install it (it ships with the CLI client) or unset the " + - "proxy env var.", - { cause }, - ); - }, - ); - return dispatcherPromise; - }; - - const proxiedFetch: typeof fetch = async (input, init) => { - const dispatcher = await getDispatcher(); - return baseFetch(input, { ...init, dispatcher } as NodeRequestInit); - }; - return proxiedFetch; -} +import { createProxyFetch } from "./proxyFetch.js"; /** * Build the wire `headers` record from `settings.headers`, dropping rows with @@ -112,9 +51,12 @@ export function createTransportNode( interceptAuthChallenges = false, } = options; - // Proxy-wrap first so the auth-challenge interceptor observes responses from - // the (optionally proxied) network call. - const baseFetch = withProxyDispatcher(optionsFetchFn ?? globalThis.fetch); + // `optionsFetchFn` is the caller's whole fetch stack and already sits on top of + // a proxy-aware base when one is needed — the Node clients install + // `createProxyFetch()` as `environment.fetch`, and `InspectorClient` wraps + // that. The fallback is for the one caller that supplies nothing: the web + // backend (`core/mcp/remote/node/server.ts`), which calls this directly. + const baseFetch = optionsFetchFn ?? createProxyFetch() ?? globalThis.fetch; const fetchWithOptionalAuthIntercept = interceptAuthChallenges ? createAuthChallengeInterceptFetch(baseFetch) : baseFetch; diff --git a/package.json b/package.json index 99113852..9d40a481 100644 --- a/package.json +++ b/package.json @@ -38,9 +38,10 @@ "build:tui": "cd clients/tui && npm run build", "build:web": "cd clients/web && npm run build", "build:launcher": "cd clients/launcher && npm run build", - "ci": "npm run validate && npm run coverage && npm run verify:build-gate && npm run smoke && npm run ci:storybook", + "ci": "npm run validate && npm run coverage && npm run verify:build-gate && npm run verify:bundle-externals && npm run smoke && npm run ci:storybook", "ci:storybook": "cd clients/web && npx playwright install chromium && npm run test:storybook", "verify:build-gate": "node scripts/verify-build-gate.mjs", + "verify:bundle-externals": "node scripts/verify-bundle-externals.mjs", "verify:typecheck-coverage": "node scripts/verify-typecheck-coverage.mjs", "test:scripts": "node --test \"scripts/**/*.test.mjs\"", "validate": "npm run verify:format-coverage && npm run verify:typecheck-coverage && npm run verify:dep-lockstep && npm run test:scripts && npm run validate:core && npm run validate:web && npm run validate:cli && npm run validate:tui && npm run validate:launcher", diff --git a/scripts/verify-bundle-externals.mjs b/scripts/verify-bundle-externals.mjs new file mode 100644 index 00000000..72586030 --- /dev/null +++ b/scripts/verify-bundle-externals.mjs @@ -0,0 +1,173 @@ +/** + * Guards the mirror of the "must be bundled" rule: a dependency a client + * declares **external** must still be external in the built bundle. + * + * AGENTS.md has a rule for dependencies that MUST be inlined — the + * React-rendering ones (#1952) — and nothing at all for the opposite direction, + * which is how #2067 shipped. `undici` was declared only in the root and + * `clients/cli` manifests, and tsup auto-externalizes just what the *nearest* + * manifest declares, so esbuild inlined 1.05 MB of it into the web and TUI + * bundles. Inlining a CommonJS package into an ESM bundle is not merely wasteful: + * esbuild rewrites `import("undici")` to a relative chunk whose `require("assert")` + * hits its `__require` shim and throws `Dynamic require of "assert" is not + * supported`. Because the specifier was rewritten at build time, no user-side + * install can ever satisfy it — the package is unloadable by construction, and + * every proxied connection failed with a message telling users to install a + * package that was already there. + * + * The check is deliberately on the **built output**, not the config: the config + * is what a reviewer reads, but the emitted chunk is what ships, and the two + * disagreed for four releases. It is also derived from each client's own + * `external` list rather than a list maintained here, so a newly externalized + * package is covered without editing this file. + * + * Nothing else catches this class. The unit and integration tests run against + * source with a real `undici` on the resolution path; the smokes run the built + * tree but never set a proxy env var, so the lazy `import()` never executes. + */ + +import { readdirSync, readFileSync, existsSync } from "node:fs"; +import { dirname, join, resolve } from "node:path"; +import { fileURLToPath } from "node:url"; + +const repoRoot = resolve(dirname(fileURLToPath(import.meta.url)), ".."); + +/** + * Clients that ship a tsup bundle, with the config to read `external` from and + * the build directory to inspect. `clients/launcher` is plain `tsc` — it emits + * no bundle and inlines nothing — so it has nothing to check. + */ +export const BUNDLED_CLIENTS = [ + { + name: "web", + config: "clients/web/tsup.runner.config.ts", + build: "clients/web/build", + }, + { + name: "cli", + config: "clients/cli/tsup.config.ts", + build: "clients/cli/build", + }, + { + name: "tui", + config: "clients/tui/tsup.config.ts", + build: "clients/tui/build", + }, +]; + +/** + * Extracts the string literals of a tsup config's `external: [...]` array. + * + * Reading the config as *text* rather than importing it keeps this guard free of + * the client's own toolchain (tsup, esbuild, and — for the TUI — a plugin that + * reads `ink-form` off disk at module load). Only bare string entries are + * collected; a regex entry such as `/^@inspector\/core/` is a `noExternal` + * pattern in practice and has no package name to look for. + */ +export function parseExternals(source) { + const start = source.indexOf("external: ["); + if (start === -1) return []; + let depth = 0; + let end = -1; + for (let i = source.indexOf("[", start); i < source.length; i++) { + if (source[i] === "[") depth++; + else if (source[i] === "]") { + depth--; + if (depth === 0) { + end = i; + break; + } + } + } + if (end === -1) return []; + const body = source.slice(start, end); + // Strip comments first: the entries are documented at length, and a package + // name quoted inside prose would otherwise read as an entry. + const withoutComments = body + .replace(/\/\*[\s\S]*?\*\//g, "") + .replace(/\/\/[^\n]*/g, ""); + return [...withoutComments.matchAll(/"([^"]+)"|'([^']+)'/g)] + .map((m) => m[1] ?? m[2]) + .filter((name) => name !== "external: ["); +} + +/** + * The chunk esbuild emits for an inlined package is named after the package's + * last path segment plus a hash — `undici-HXPKCIY3.js`, + * `@scope/name` → `name-XXXXXXXX.js` — and the rewritten specifier in the entry + * is the matching relative path. Either one present means the package was + * inlined despite being declared external. + */ +export function findInlinedExternals({ externals, buildFiles, entrySource }) { + const violations = []; + for (const pkg of externals) { + const base = pkg.split("/").pop(); + const chunk = buildFiles.find((f) => + new RegExp(`^${escapeRegExp(base)}-[A-Za-z0-9_]+\\.js$`).test(f), + ); + if (chunk) { + violations.push({ pkg, reason: `emitted chunk ${chunk}` }); + continue; + } + if ( + entrySource.includes(`"./${base}-`) || + entrySource.includes(`'./${base}-`) + ) { + violations.push({ + pkg, + reason: "entry imports a rewritten relative chunk", + }); + } + } + return violations; +} + +function escapeRegExp(s) { + return s.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); +} + +function main() { + const failures = []; + for (const client of BUNDLED_CLIENTS) { + const buildDir = join(repoRoot, client.build); + const entry = join(buildDir, "index.js"); + if (!existsSync(entry)) { + failures.push( + `${client.name}: ${client.build}/index.js is missing — run \`npm run build\` first.`, + ); + continue; + } + const externals = parseExternals( + readFileSync(join(repoRoot, client.config), "utf8"), + ); + const violations = findInlinedExternals({ + externals, + buildFiles: readdirSync(buildDir), + entrySource: readFileSync(entry, "utf8"), + }); + for (const v of violations) { + failures.push( + `${client.name}: "${v.pkg}" is declared external but was inlined (${v.reason}).`, + ); + } + } + + if (failures.length > 0) { + console.error("verify:bundle-externals FAILED\n"); + for (const f of failures) console.error(` - ${f}`); + console.error( + "\nAn externalized package must resolve from the consumer's install. If it was\n" + + "inlined, esbuild rewrote its specifier to a relative chunk that no user-side\n" + + "install can satisfy — and a CommonJS package inlined into an ESM bundle throws\n" + + '`Dynamic require of "..." is not supported` on first use (#2067).\n' + + "Add the package to that client's tsup `external` list.", + ); + process.exit(1); + } + + console.log( + `verify:bundle-externals OK — ${BUNDLED_CLIENTS.length} bundles, no externalized package inlined.`, + ); +} + +if (process.argv[1] === fileURLToPath(import.meta.url)) main(); diff --git a/scripts/verify-bundle-externals.test.mjs b/scripts/verify-bundle-externals.test.mjs new file mode 100644 index 00000000..ac443813 --- /dev/null +++ b/scripts/verify-bundle-externals.test.mjs @@ -0,0 +1,122 @@ +/** + * Table-driven tests for the pure parsers of `verify-bundle-externals.mjs`. + * + * One case per rule the guard encodes. The guard itself reads real build output, + * which no unit test can produce cheaply — so what is pinned here is the + * *decision* logic: which entries count as externals, and which emitted shapes + * count as "inlined anyway". + */ + +import { test, describe } from "node:test"; +import assert from "node:assert/strict"; + +import { + BUNDLED_CLIENTS, + parseExternals, + findInlinedExternals, +} from "./verify-bundle-externals.mjs"; + +describe("parseExternals", () => { + test("collects bare string entries", () => { + assert.deepEqual( + parseExternals(`export default { external: ["undici", "pino"] }`), + ["undici", "pino"], + ); + }); + + test("ignores package names that appear only inside comments", () => { + // The real configs document each entry at length, and #2067's entry names + // `undici` in its own prose — a comment-blind scan would double-count it and, + // worse, invent entries for packages merely discussed. + const src = `export default { external: [ + // "ghost" is explained here but is NOT an entry. + /* neither is "phantom" */ + "undici", + ] }`; + assert.deepEqual(parseExternals(src), ["undici"]); + }); + + test("handles single quotes and nested arrays", () => { + assert.deepEqual( + parseExternals( + `export default { external: ['a', 'b'], noExternal: ["c"] }`, + ), + ["a", "b"], + ); + }); + + test("returns an empty list when there is no external array", () => { + assert.deepEqual(parseExternals(`export default { entry: ["x.ts"] }`), []); + }); +}); + +describe("findInlinedExternals", () => { + test("flags an emitted chunk named after the package", () => { + // The exact shape #2067 shipped: `undici-HXPKCIY3.js` beside index.js. + const v = findInlinedExternals({ + externals: ["undici"], + buildFiles: ["index.js", "undici-HXPKCIY3.js"], + entrySource: "", + }); + assert.equal(v.length, 1); + assert.equal(v[0].pkg, "undici"); + assert.match(v[0].reason, /undici-HXPKCIY3\.js/); + }); + + test("flags a rewritten relative specifier in the entry", () => { + // Belt to the chunk-name brace: if esbuild ever names the chunk differently, + // the rewritten import in the entry still gives the inlining away. + const v = findInlinedExternals({ + externals: ["undici"], + buildFiles: ["index.js"], + entrySource: 'await import("./undici-ABC123.js")', + }); + assert.equal(v.length, 1); + assert.match(v[0].reason, /rewritten relative chunk/); + }); + + test("matches a scoped package on its last segment", () => { + const v = findInlinedExternals({ + externals: ["@napi-rs/keyring"], + buildFiles: ["index.js", "keyring-QQ11.js"], + entrySource: "", + }); + assert.equal(v.length, 1); + assert.equal(v[0].pkg, "@napi-rs/keyring"); + }); + + test("passes a genuinely external package", () => { + assert.deepEqual( + findInlinedExternals({ + externals: ["undici", "pino"], + buildFiles: ["index.js"], + entrySource: 'await import("undici"); import "pino";', + }), + [], + ); + }); + + test("does not mistake an unrelated chunk for the package", () => { + // `undici-types` is a @types transitive; a substring match would flag it. + assert.deepEqual( + findInlinedExternals({ + externals: ["undici"], + buildFiles: ["index.js", "undici-types-AA22.js"], + entrySource: "", + }), + [], + ); + }); +}); + +describe("BUNDLED_CLIENTS", () => { + test("covers every tsup-bundled client", () => { + // `clients/launcher` is plain tsc and emits no bundle, so it is deliberately + // absent. If a fourth bundled client appears, it must be enrolled here. + assert.deepEqual(BUNDLED_CLIENTS.map((c) => c.name).sort(), [ + "cli", + "tui", + "web", + ]); + }); +});