diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 63290884..49a1318e 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -101,7 +101,7 @@ Both exist and do different jobs. Theme files (`src/theme/.ts`) custo ## Gates and PR hygiene -- `npm run format` before committing; **`npm run ci` before pushing** (`validate` → `coverage` → `verify:build-gate` → `smoke` → Storybook). `npm run validate` is the fast inner-loop check and is **not** a substitute — it runs `test`, not `test:coverage`, so it does zero coverage gating. +- `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_. diff --git a/AGENTS.md b/AGENTS.md index 04f870d9..4a4b142e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -253,7 +253,7 @@ The same **placement** rule covers anything reached only through **root-owned co **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: +**`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. 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. 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. @@ -795,7 +795,7 @@ 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`. @@ -804,7 +804,7 @@ The ⚠️ option-deletion hazard, the snapshot rule, and the recovery recipe ab - **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 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` 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. diff --git a/README.md b/README.md index d1afcbab..25a2555d 100644 --- a/README.md +++ b/README.md @@ -367,12 +367,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 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: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. 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 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. diff --git a/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts b/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts index e6140467..795e7e3c 100644 --- a/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts +++ b/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts @@ -341,6 +341,62 @@ describe("server.ts supplemental coverage", () => { await stop(h); }); + it("routes the outbound request through HTTP_PROXY (#2067)", async () => { + // /api/fetch is the browser's ONLY way out to the network — the web + // client's `environment.fetch` is `createRemoteFetch()`, which forwards + // OAuth discovery and token requests here. On the bare global `fetch` a + // corporate-proxy user could connect to a server but never authorize + // against it, and Node's native NODE_USE_ENV_PROXY does not cover them + // (unsupported at the 22.19 engine floor). + const { createServer } = await import("node:http"); + const { request } = await import("node:http"); + const seen: string[] = []; + const proxy = createServer((req, res) => { + seen.push(req.url ?? ""); + const u = new URL(req.url ?? ""); + const up = request( + { + host: u.hostname, + port: u.port, + path: u.pathname + u.search, + method: req.method, + headers: req.headers, + }, + (r) => { + res.writeHead(r.statusCode ?? 502, r.headers); + r.pipe(res); + }, + ); + up.on("error", () => { + res.writeHead(502); + res.end(); + }); + req.pipe(up); + }); + await new Promise((r) => proxy.listen(0, "127.0.0.1", () => r())); + const proxyAddr = proxy.address(); + const proxyPort = + typeof proxyAddr === "object" && proxyAddr !== null + ? proxyAddr.port + : 0; + const previous = process.env.HTTP_PROXY; + process.env.HTTP_PROXY = `http://127.0.0.1:${proxyPort}`; + + try { + const res = await fetch(`${h.baseUrl}/api/fetch`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ url: `${targetUrl}/plain` }), + }); + expect(res.status).toBe(200); + expect(seen).toEqual([`${targetUrl}/plain`]); + } finally { + if (previous === undefined) delete process.env.HTTP_PROXY; + else process.env.HTTP_PROXY = previous; + await new Promise((r) => proxy.close(() => r())); + } + }); + it("forwards method + headers and returns the response body", async () => { const res = await fetch(`${h.baseUrl}/api/fetch`, { method: "POST", diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index 6f778c66..b1f932ad 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -26,6 +26,7 @@ import type { Context, Env, Next } from "hono"; import { streamSSE } from "hono/streaming"; import { watch as chokidarWatch, type FSWatcher } from "chokidar"; import { createTransportNode } from "../../node/transport.js"; +import { createProxyFetch } from "../../node/proxyFetch.js"; import type { RemoteConnectRequest, RemoteSendRequest, @@ -1054,7 +1055,14 @@ export function createRemoteApp( } try { - const res = await fetch(url, { + // Proxy-aware, not the bare global. This route is the browser's ONLY way + // out to the network: the web client's `environment.fetch` is + // `createRemoteFetch()`, which forwards OAuth discovery and token + // requests here. Left on the global `fetch`, a corporate-proxy user could + // connect to a server but never authorize against it — and Node's native + // `NODE_USE_ENV_PROXY` does not cover them either, being unsupported at + // our 22.19 engine floor (#2067). + const res = await (createProxyFetch() ?? fetch)(url, { method, headers: new Headers(headers), body: reqBody, diff --git a/scripts/verify-bundle-externals.mjs b/scripts/verify-bundle-externals.mjs index 72586030..2b245e2b 100644 --- a/scripts/verify-bundle-externals.mjs +++ b/scripts/verify-bundle-externals.mjs @@ -92,38 +92,45 @@ export function parseExternals(source) { } /** - * 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. + * Every module esbuild inlines is emitted under a `// ` banner naming its + * source file, so a package that was bundled despite being declared external + * leaves `// ../../node_modules//…` lines behind. Matching on those covers + * **both** shapes this can take: a separate `-HASH.js` chunk (what a + * dynamically `import()`ed CommonJS package produces, which is what #2067 was) + * and a statically-imported package folded straight into `index.js`, which + * emits no chunk at all and would otherwise pass unnoticed. + * + * Reading banners means the guard depends on unminified output. That is true of + * every client bundle today and is asserted rather than assumed: a build with no + * banners at all fails as `noBanners` instead of silently reporting a clean + * result, so turning on `minify` surfaces here rather than quietly retiring the + * check. */ -export function findInlinedExternals({ externals, buildFiles, entrySource }) { +export function findInlinedExternals({ externals, files }) { + const banners = files.flatMap((file) => + [...file.source.matchAll(/^\/\/ (\S+)$/gm)].map((m) => ({ + file: file.name, + path: m[1], + })), + ); + if (banners.length === 0) { + return { noBanners: true, violations: [] }; + } + 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}-`) - ) { + // The trailing slash is load-bearing: without it `undici` matches + // `undici-types`, a @types transitive that is legitimately present. + const needle = `node_modules/${pkg}/`; + const hit = banners.find((b) => b.path.includes(needle)); + if (hit) { violations.push({ pkg, - reason: "entry imports a rewritten relative chunk", + reason: `inlined into ${hit.file} (e.g. ${hit.path})`, }); } } - return violations; -} - -function escapeRegExp(s) { - return s.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); + return { noBanners: false, violations }; } function main() { @@ -140,11 +147,23 @@ function main() { const externals = parseExternals( readFileSync(join(repoRoot, client.config), "utf8"), ); - const violations = findInlinedExternals({ + const files = readdirSync(buildDir) + .filter((f) => f.endsWith(".js")) + .map((name) => ({ + name, + source: readFileSync(join(buildDir, name), "utf8"), + })); + const { noBanners, violations } = findInlinedExternals({ externals, - buildFiles: readdirSync(buildDir), - entrySource: readFileSync(entry, "utf8"), + files, }); + if (noBanners) { + failures.push( + `${client.name}: ${client.build} has no esbuild module banners, so this guard ` + + `cannot see what was inlined. Has minification been enabled for this bundle?`, + ); + continue; + } for (const v of violations) { failures.push( `${client.name}: "${v.pkg}" is declared external but was inlined (${v.reason}).`, diff --git a/scripts/verify-bundle-externals.test.mjs b/scripts/verify-bundle-externals.test.mjs index ac443813..bac8b53b 100644 --- a/scripts/verify-bundle-externals.test.mjs +++ b/scripts/verify-bundle-externals.test.mjs @@ -51,61 +51,104 @@ describe("parseExternals", () => { }); 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({ + const banner = (p) => `// ${p}\nvar x = 1;\n`; + + test("flags a package inlined as its own chunk", () => { + // The exact shape #2067 shipped: a dynamically import()ed CommonJS package + // emitted as `undici-HXPKCIY3.js` beside index.js. + const { violations } = findInlinedExternals({ externals: ["undici"], - buildFiles: ["index.js", "undici-HXPKCIY3.js"], - entrySource: "", + files: [ + { + name: "index.js", + source: banner("../../core/mcp/node/transport.ts"), + }, + { + name: "undici-HXPKCIY3.js", + source: banner("../../node_modules/undici/lib/core/symbols.js"), + }, + ], }); - assert.equal(v.length, 1); - assert.equal(v[0].pkg, "undici"); - assert.match(v[0].reason, /undici-HXPKCIY3\.js/); + assert.equal(violations.length, 1); + assert.equal(violations[0].pkg, "undici"); + assert.match(violations[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")', + test("flags a package inlined straight into the entry, with no chunk", () => { + // A *statically* imported package is folded into index.js and emits no + // chunk at all, so a chunk-name check would miss it entirely. The banner is + // what gives it away. + const { violations } = findInlinedExternals({ + externals: ["pino"], + files: [ + { + name: "index.js", + source: + banner("../../core/mcp/index.ts") + + banner("../../node_modules/pino/pino.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"); + assert.equal(violations.length, 1); + assert.equal(violations[0].pkg, "pino"); + assert.match(violations[0].reason, /index\.js/); }); test("passes a genuinely external package", () => { - assert.deepEqual( - findInlinedExternals({ - externals: ["undici", "pino"], - buildFiles: ["index.js"], - entrySource: 'await import("undici"); import "pino";', - }), - [], - ); + const { noBanners, violations } = findInlinedExternals({ + externals: ["undici", "pino"], + files: [ + { + name: "index.js", + source: + banner("../../core/mcp/node/proxyFetch.ts") + + 'await import("undici");\nimport "pino";\n', + }, + ], + }); + assert.equal(noBanners, false); + assert.deepEqual(violations, []); }); - 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: "", - }), - [], - ); + test("does not mistake a same-prefix package for the external", () => { + // `undici-types` is a @types transitive and is legitimately inlined; the + // trailing slash in the needle is what keeps it from reading as `undici`. + const { violations } = findInlinedExternals({ + externals: ["undici"], + files: [ + { + name: "index.js", + source: banner("../../node_modules/undici-types/index.js"), + }, + ], + }); + assert.deepEqual(violations, []); + }); + + test("matches a scoped package on its full name", () => { + const { violations } = findInlinedExternals({ + externals: ["@napi-rs/keyring"], + files: [ + { + name: "index.js", + source: banner("../../node_modules/@napi-rs/keyring/index.js"), + }, + ], + }); + assert.equal(violations.length, 1); + assert.equal(violations[0].pkg, "@napi-rs/keyring"); + }); + + test("reports noBanners rather than a clean pass when banners are absent", () => { + // The guard reads esbuild's `// ` module banners, so minified output + // would make every bundle look clean. That must fail loudly instead — + // otherwise enabling `minify` silently retires the check. + const { noBanners, violations } = findInlinedExternals({ + externals: ["undici"], + files: [{ name: "index.js", source: "var a=1;var b=2;" }], + }); + assert.equal(noBanners, true); + assert.deepEqual(violations, []); }); });