modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

Merge branch 'v2/main' into v2/feat/2082-file-secret-store-lock

Browse source
This commit is contained in:
Cliff Hall 2026-08-23 18:09:04 -04:00 • committed by GitHub
commit 89b01d01ff
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
7 changed files with 343 additions and 87 deletions
Download patch file Download diff file Expand all files Collapse all files

10
AGENTS.md
View file

@ -206,7 +206,13 @@ v2/main/
│ # pack-and-verify.mjs, and lib/ shared helpers
│ # (tsc-program.mjs is the `tsc --listFilesOnly`
│ # measurement both coverage guards read a program
│ # through — #1965; resolve-node-bin.mjs resolves a
│ # through — #1965; announced-child.mjs owns the
│ # spawn-and-wait-for-a-readiness-line step, publishing
│ # the child via `onSpawn` BEFORE the wait so the
│ # caller's teardown reaches it on every throw path —
│ # the readiness timeout included, which is what used
│ # to orphan a live server holding its port — #2000;
│ # resolve-node-bin.mjs resolves a
│ # package's bin through its own package.json, so the
│ # verify/smoke scripts spawn it with `process.execPath`
│ # rather than the `npx` `.cmd` shim Node refuses to
@ -796,7 +802,7 @@ The ⚠️ option-deletion hazard, the snapshot rule, and the recovery recipe ab
**Every web smoke runs against a throwaway catalog (#1977).** The helper mints a temp dir per run and passes it as `MCP_CATALOG_PATH`; without it the web backend falls back to the developer's real `~/.mcp-inspector/mcp.json`, which made these smokes both destructive and non-deterministic — `smoke:web:app`'s deep link persists a `deep-link` server row, so a *second* run found it already on disk, raced hydration, and drew a spurious (swallowed, non-fatal) 409 that was really just residue from the previous run. CI never saw it: a fresh `HOME` per run made every CI run look like a first run. This matches `smoke:cli` / `smoke:tui`, which have always driven a temp `--catalog`. Only the **catalog** is redirected — other per-user state under `~/.mcp-inspector` (OAuth tokens, `storage/`) stays shared, because isolating it means redirecting `HOME` wholesale, which would also move the npx and Playwright caches these smokes depend on. Teardown uses **both** halves of `scripts/lib/child-cleanup.mjs` (`stopChild` to await the child's exit, then `removeSafe` to delete the dir) — a bare `kill()` only *delivers* the signal, so removing synchronously re-enters the #1801 ENOTEMPTY race. That makes `stop()` **async**, so every caller must `await` it (and a caller's own `fail()`/`shutdown()` becomes async in turn, or execution runs past the intended exit). The isolation contract is unit-tested in `scripts/lib/prod-web-server.test.mjs` via `test:scripts`, since the smokes exit immediately after teardown and so cannot detect a regression that silently reshared the catalog or stopped cleaning up: `createTempCatalog` and `buildWebServerEnv` cover *which* catalog the server gets, and `teardownWebServer` — extracted from `stop()` for exactly this reason — is driven against a stand-in child process so the teardown asserts on the real directory rather than a spy.
- `smoke:web:browser` (`scripts/smoke-web-browser.mjs`, #1615) goes a step further than `smoke:web`: it boots the same prod `--web` server and then actually **runs** the bundle in headless Chromium (Playwright — already a `clients/web` devDependency for the Storybook tests), asserting the app renders its first meaningful frame (the "Add Servers" control) with **no uncaught error**. `smoke:web` only checks the served HTML, so a Node built-in reaching the browser bundle slipped through it; this smoke catches that regression as a _class_ (e.g. #1612). The mechanism is the uncaught error, not a magic string: under Vite the excluded module becomes an empty stub and the first _call_ into it (e.g. `fs.readFileSync(...)` during a transitive module's init) throws a `TypeError` that aborts app mount. A _synchronous_ such throw fires `pageerror`; its _async_ twin (the same `TypeError` via `await`/`.then()`, or a failed dynamic import) is logged on the console channel as `Uncaught (in promise) …` / `Failed to fetch dynamically imported module` — the smoke hard-fails on both. The literal `Module "…" has been externalized` text is, **in a prod build**, a build-time warning (`vite build` / `npm run build`), not a runtime message, so the browser never sees it (under `npm run dev` Vite's stub is instead a `Proxy` that `console.warn`s that string at runtime); and an externalized import that is never _called_ ships a harmless `{}` and is invisible here by design. Every _other_ console error is printed as a diagnostic, not a failure (so a benign font-CDN or React-warning `console.error` doesn't flake CI). Playwright is resolved via `createRequire` based at `clients/web/package.json` — a bare `import("playwright")` would resolve relative to `scripts/`, not the cwd, so it can't be reached that way (it only appears to work when an ancestor `node_modules` carries playwright, and fails in CI, which has none). The npm script's `cd clients/web` exists only so `npx playwright install chromium` finds the local playwright bin (a no-op when already installed).
- `smoke:web:app` (`scripts/smoke-web-app.mjs`, #1859) goes one step further again: `smoke:web:browser` stops at first paint and never connects to a server, so the Apps tab, the sandbox controller, and the UI-protocol bridge were unexercised by any smoke. This one boots the same prod `--web` server, spawns the `mcp-app-http.json` composable test server (the `mcp_app_demo` tool + its `mcp_app_demo_widget` UI resource), and drives the whole **connect → open app → widget ready** chain through a single deep-link navigate (`?serverUrl=…&autoConnect=<token>&openApp=…&appArgs=…&autoOpen=<token>`). The assertion is the `data-app-status="ready"` contract from [clients/web/README.md](clients/web/README.md) — the renderer reports `ready` only once the widget has loaded inside the sandbox iframe _and_ fired `notifications/initialized` back through the bridge, so one attribute covers the sandbox proxy being served, the UI resource loading, and the handshake completing. Two mechanics are load-bearing and easy to get wrong: the test server announces readiness on **stderr** (`console.error` in `server-composable.ts`), so both child streams are piped and scanned — watching stdout alone times out with an empty diagnostic; and its bound port is **not** the config's, because `createTestServerHttp` resolves through `findAvailablePort()`, which walks upward when the configured port is taken — so the smoke parses the announced URL rather than assuming `3130`. **Scope note:** this runs against the repo build tree like every other smoke, so it would _not_ have caught #1859 itself (a packaging failure — the file is always present in-repo); `pack:verify` owns that dimension. It does carry a cheap structural pre-check that the proxy page exists at the path `sandbox-controller.ts` resolves, so a move/rename fails fast with a clear cause instead of an opaque render timeout.
- `smoke:web:app` (`scripts/smoke-web-app.mjs`, #1859) goes one step further again: `smoke:web:browser` stops at first paint and never connects to a server, so the Apps tab, the sandbox controller, and the UI-protocol bridge were unexercised by any smoke. This one boots the same prod `--web` server, spawns the `mcp-app-http.json` composable test server (the `mcp_app_demo` tool + its `mcp_app_demo_widget` UI resource), and drives the whole **connect → open app → widget ready** chain through a single deep-link navigate (`?serverUrl=…&autoConnect=<token>&openApp=…&appArgs=…&autoOpen=<token>`). The assertion is the `data-app-status="ready"` contract from [clients/web/README.md](clients/web/README.md) — the renderer reports `ready` only once the widget has loaded inside the sandbox iframe _and_ fired `notifications/initialized` back through the bridge, so one attribute covers the sandbox proxy being served, the UI resource loading, and the handshake completing. Two mechanics are load-bearing and easy to get wrong: the test server announces readiness on **stderr** (`console.error` in `server-composable.ts`), so both child streams are piped and scanned — watching stdout alone times out with an empty diagnostic; and its bound port is **not** the config's, because `createTestServerHttp` resolves through `findAvailablePort()`, which walks upward when the configured port is taken — so the smoke parses the announced URL rather than assuming `3130`. Both mechanics now live in `scripts/lib/announced-child.mjs` rather than in the smoke, so the failure path is testable: this smoke's happy path always receives the announcement, so nothing it could assert would prove that a child alive *through* the 30s timeout is still reachable by `shutdown()` — the case that orphaned a live server (#2000). The helper publishes the child via `onSpawn` before waiting, and `scripts/lib/announced-child.test.mjs` drives real `node -e` children (not spies) to assert it is published, still alive when the throw lands, and actually killable. Same reason `teardownWebServer` was extracted from `prod-web-server.mjs`'s `stop()`. **Scope note:** this runs against the repo build tree like every other smoke, so it would _not_ have caught #1859 itself (a packaging failure — the file is always present in-repo); `pack:verify` owns that dimension. It does carry a cheap structural pre-check that the proxy page exists at the path `sandbox-controller.ts` resolves, so a move/rename fails fast with a clear cause instead of an opaque render timeout.
- `smoke:web:elicit` (`scripts/smoke-web-elicitation.mjs`, #1854) is the app-rendered **elicitation** counterpart of `smoke:web:app`: same prod `--web` server and the same deep-link connect, but it then calls `app_choose_option` from the Tools tab, waits for `[data-testid="app-elicitation"][data-app-elicitation-status="ready"]`, clicks a choice **inside the sandboxed app** (two `frameLocator` hops — the trusted sandbox proxy, then the untrusted app), and asserts the app's standard `ElicitResult` comes back in the *tool result*, i.e. that it reached the server rather than merely the host. It then repeats against `app-elicitation-native-http.json` — the same tool and app on a server that never advertised the nested MCP Apps `elicitation` capability — and asserts the **native** elicitation dialog takes it and no app modal is rendered. That second half is the more valuable one: the failure this feature can produce is not "the app doesn't render" but "an app renders when it should not have been offered one", which strands every user of a server that never opted in. Set `SMOKE_SCREENSHOT_DIR` to capture PNGs of the three states (used for PR proof); unset, it asserts only. Two mechanics worth knowing: the main-view tabs are a Mantine `SegmentedControl`, so there is no `role="tab"` — the clickable element is the sibling `label[for$="-Tools"]`; and the prompt string also appears in the (hidden) Protocol-tab payload, so the fallback assertion is scoped to the dialog rather than a bare text lookup.
- **The build gate for the browser-externalized-builtin class (#1769)** is the earlier, more complete companion to `smoke:web:browser`. A Vite plugin in `clients/web/vite.config.ts` (logic in `clients/web/server/browser-externalized-builtin-gate.ts`, unit-tested) turns Vite 8's _browser-externalization warning_ (`Module "node:*" has been externalized for browser compatibility`) into a hard `vite build` error, so a Node built-in in the browser graph now **fails `npm run build` / `validate`** instead of shipping a `{}` stub. This catches **both** the _called-at-init_ case (which `smoke:web:browser` also catches, but later/at runtime) **and** the _imported-but-never-called_ case (the `{}` stub that is invisible to the runtime smoke "by design" — see above). Because rolldown **swallows a throw inside `onLog`** (the one hook where a thrown error doesn't abort — verified against vite@8.0.0), the plugin _records_ the warning in `onLog` and re-throws in `buildEnd`. There is **no stable log `code`**, so the gate keys off the documented message phrasing; `npm run verify:build-gate` (`scripts/verify-build-gate.mjs`, in `npm run ci` and the GitHub workflow) runs a real build with a `node:fs` probe forced into `src/main.tsx` and asserts the build fails via the gate — the only check that catches the message phrasing **drifting** in a future Vite bump and silently disabling the gate. The gate is scoped to `vite build` (`apply: 'build'`) — never `vite dev` or the vitest projects — **and** to the browser (`client`) environment (`applyToEnvironment`), so a future SSR/node environment built from this config isn't failed for a legitimate `node:*` import; the Node runner build (tsup, `build:runner`) is a separate config where built-ins are legitimate. `smoke:web:browser` stays as the runtime backstop for crashes the build can't reason about.

2
README.md
View file

@ -368,7 +368,7 @@ Each client self-validates from its own folder; the root scripts chain them. The
| `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: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 `scripts/lib/resolve-node-bin.test.mjs` — the cross-platform bin resolver (#1939), pinned against the real `bin`/`exports` shapes of the packages the scripts actually spawn. Runs in `validate` — and `verify:typecheck-coverage` guards *this* gate in turn (reachable from `validate`, non-empty test set, every test file matched by the `test:scripts` glob), since `node --test` silently skips a file its glob misses and still exits 0. |
| `npm run 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. |

14
clients/web/.npmignore
View file

@ -8,11 +8,15 @@
# load. (The other clients don't hit this because none of them ship a nested
# .gitignore.)
#
# Crucially this file does NOT list `build` or `dist`, so both the prod runner
# (build/) and the SPA (dist/) are packed. The root "files" allowlist already
# restricts publishing to those two directories, so everything else in
# clients/web (src, configs, node_modules, coverage, storybook-static) stays out
# regardless — the entries below are just belt-and-suspenders.
# Crucially this file does NOT list `build`, `dist`, or `static`, so all three
# are packed: the prod runner (build/), the SPA (dist/), and the MCP Apps
# sandbox proxy page (static/sandbox_proxy.html — a committed source file, read
# from disk at runtime by server/sandbox-controller.ts as
# `<runner dir>/../static/sandbox_proxy.html`, so it must ship at exactly that
# path; #1859). The root "files" allowlist names those three directories and
# nothing else under clients/web, so the rest (src, configs, node_modules,
# coverage, storybook-static) stays out regardless — the entries below are just
# belt-and-suspenders.
node_modules
coverage
storybook-static

80
clients/web/src/test/core/react/useServers.test.tsx
View file

@ -875,7 +875,9 @@ describe("useServers", () => {
fetchFn: async (input, init) => {
const url = input instanceof Request ? input.url : String(input);
if (url.endsWith("/api/servers/events")) {
return { ok: true, body: null } as unknown as Response;
// A real Response constructed from `null` has a null `.body`,
// so the guard is exercised through the actual Response API.
return new Response(null, { status: 200 });
}
return h.fetchFn(url, init);
},
@ -894,12 +896,14 @@ describe("useServers", () => {
fetchFn: async (input, init) => {
const url = input instanceof Request ? input.url : String(input);
if (url.endsWith("/api/servers/events")) {
const body = {
getReader: () => ({
read: () => Promise.reject(new Error("stream broke")),
}),
};
return { ok: true, body } as unknown as Response;
// A real stream whose first pull throws — `reader.read()` then
// rejects exactly as a broken network body would, with no cast.
const body = new ReadableStream<Uint8Array>({
pull() {
throw new Error("stream broke");
},
});
return new Response(body, { status: 200 });
}
return h.fetchFn(url, init);
},
@ -928,22 +932,18 @@ describe("useServers", () => {
fetchFn: async (input, init) => {
const url = input instanceof Request ? input.url : String(input);
if (url.endsWith("/api/servers/events")) {
const body = {
getReader: () => ({
read: async () => {
reads += 1;
if (reads === 1) {
// Two frames in one chunk → one background refresh.
return {
done: false,
value: encoder.encode("event: change\n\n\n\n"),
};
}
return { done: true, value: undefined };
},
}),
};
return { ok: true, body } as unknown as Response;
const body = new ReadableStream<Uint8Array>({
pull(controller) {
reads += 1;
if (reads === 1) {
// Two frames in one chunk → one background refresh.
controller.enqueue(encoder.encode("event: change\n\n\n\n"));
return;
}
controller.close();
},
});
return new Response(body, { status: 200 });
}
return h.fetchFn(url, init);
},
@ -1138,22 +1138,24 @@ describe("useServers", () => {
const fetchFn: typeof fetch = async (input, init) => {
const url = input instanceof Request ? input.url : String(input);
if (url.endsWith("/api/servers/events")) {
const body = {
getReader: () => ({
read: async () => {
reads += 1;
// Priming comment only — no `event:` / `data:` line.
if (reads === 1) {
return { done: false, value: encoder.encode(":\n\n") };
}
// Hold the stream open so the loop can't end and let a
// teardown-time settle hide a queued refresh.
await secondRead;
return { done: true, value: undefined };
},
}),
};
return { ok: true, body } as unknown as Response;
// `pull` runs once per read, so the counting and the blocking
// second read work the same way they would on a hand-rolled reader
// double — while staying type-checked against the Response API.
const body = new ReadableStream<Uint8Array>({
async pull(controller) {
reads += 1;
// Priming comment only — no `event:` / `data:` line.
if (reads === 1) {
controller.enqueue(encoder.encode(":\n\n"));
return;
}
// Hold the stream open so the loop can't end and let a
// teardown-time settle hide a queued refresh.
await secondRead;
controller.close();
},
});
return new Response(body, { status: 200 });
}
if (url.endsWith("/api/servers")) listGets += 1;
return h.fetchFn(url, init);

94
scripts/lib/announced-child.mjs Normal file
View file

@ -0,0 +1,94 @@
/**
* Spawn a child process and wait for it to announce readiness on its output.
*
* Extracted from `scripts/smoke-web-app.mjs` so the failure path can be tested
* (#2000). A smoke script only ever exercises its own happy path: the real MCP
* test server always announces, so nothing in `npm run ci` proved that a child
* which stays alive *through* the readiness timeout is still reachable by the
* caller's teardown. That is precisely the case that used to orphan a live
* server holding its port, and it is invisible to the smoke itself.
*
* The ownership rule this encodes: the child is handed to `onSpawn` the moment
* it exists, **before** the readiness wait, so every throw path below leaves
* the caller holding a stoppable handle. Returning it only on success is what
* made the timeout leak — `spawnError` and `exited` were never affected (the
* child is already gone there), but they cost nothing to cover the same way.
*
* Both stdio channels are piped and scanned: a child that announces with
* `console.error` is missed entirely by a stdout-only scan, which then times
* out with an empty diagnostic. Piping both also keeps the child's noise out of
* the caller's output while still making it available in the failure message.
*/
import { spawn } from "node:child_process";
import { setTimeout as delay } from "node:timers/promises";
/**
* @param {object} opts
* @param {string} opts.command Executable to spawn.
* @param {string[]} opts.args Arguments for it.
* @param {string} [opts.cwd] Working directory.
* @param {RegExp} opts.pattern Matched against the accumulated output;
* the first match ends the wait.
* @param {(child: import("node:child_process").ChildProcess) => void} opts.onSpawn
* Called synchronously with the child immediately after spawn, before any
* waiting. This is the caller's teardown handle.
* @param {string} opts.what Noun used in error messages ("MCP test server").
* @param {number} [opts.timeoutMs] Readiness budget (default 30s).
* @param {number} [opts.pollMs] Poll interval (default 250ms).
* @returns {Promise<{ child: import("node:child_process").ChildProcess, match: RegExpMatchArray }>}
*/
export async function startAnnouncedChild({
command,
args,
cwd,
pattern,
onSpawn,
what,
timeoutMs = 30_000,
pollMs = 250,
}) {
const child = spawn(command, args, {
cwd,
stdio: ["ignore", "pipe", "pipe"],
});
// Publish before waiting — this is the whole point of the helper.
onSpawn(child);
let out = "";
child.stdout.on("data", (d) => (out += d));
child.stderr.on("data", (d) => (out += d));
let exited = false;
let spawnError = null;
// A spawn failure (e.g. an unbuilt/renamed entry) emits `error`, NOT `exit` —
// and with no `error` listener Node throws it uncaught, replacing the caller's
// diagnostic with a raw stack. `close` is listened to alongside `exit` for the
// same reason: it fires in cases `exit` does not, so a child that dies without
// an exit event can't leave the poll below spinning for the full budget.
child.on("error", (err) => (spawnError = err));
child.on("exit", () => (exited = true));
child.on("close", () => (exited = true));
// A deadline loop, not a fixed attempt count: state is re-read *after* every
// wait, including the last one. Counting attempts and throwing straight after
// the final `delay` leaves the whole last polling interval unobserved, so an
// announcement landing at 29.9s of a 30s budget is reported as a timeout —
// and a spawn error or early exit in that window is misattributed the same
// way. The final wait is also clamped to the deadline so the budget is a
// real bound rather than one poll longer.
const deadline = Date.now() + timeoutMs;
for (;;) {
const match = out.match(pattern);
if (match) return { child, match };
if (spawnError) {
throw new Error(`could not spawn the ${what}: ${spawnError.message}`);
}
if (exited) throw new Error(`${what} exited early:\n${out}`);
const remaining = deadline - Date.now();
if (remaining <= 0) break;
await delay(Math.min(pollMs, remaining));
}
throw new Error(
`${what} did not start within ${Math.round(timeoutMs / 1000)}s:\n${out}`,
);
}

163
scripts/lib/announced-child.test.mjs Normal file
View file

@ -0,0 +1,163 @@
/**
* Tests for the spawn/readiness ownership helper (#2000).
*
* The invariant under test is the one a smoke script can never check itself:
* a child that stays alive *through* the readiness timeout must still be
* reachable by the caller's teardown. `smoke:web:app`'s happy path always gets
* the announcement, so a regression here would be silent in `npm run ci` and
* would surface only as a stray process holding a port on a later run.
*
* Every case drives a real `node -e` child rather than a spy, so the assertion
* is about a real process being killable, not about a mock's call log.
*/
import test from "node:test";
import assert from "node:assert/strict";
import { startAnnouncedChild } from "./announced-child.mjs";
/** Wait for a real child to exit (or resolve immediately if it already has). */
function waitForExit(child) {
if (child.exitCode !== null || child.signalCode !== null)
return Promise.resolve();
return new Promise((resolve) => child.once("close", resolve));
}
/**
* Build an `onSpawn` that registers teardown the moment the child exists, so it
* is killed on every path out of the test — a failed assertion included.
*
* This is the helper's own ownership rule applied to the suite: killing only on
* the success path would leak a 60-second child into the runner precisely when
* the regression being guarded has come back, which is the one run where a leak
* matters most. Killing an already-exited child is a no-op, so the early-exit
* cases pay nothing for it.
*/
function killOnTeardown(t, capture = () => {}) {
return (child) => {
capture(child);
t.after(async () => {
child.kill("SIGKILL");
await waitForExit(child);
});
};
}
test("hands the child to onSpawn before waiting, and returns the match", async (t) => {
let seen = null;
const { child, match } = await startAnnouncedChild({
command: process.execPath,
args: ["-e", 'console.error("listening at http://127.0.0.1:9999")'],
pattern: /listening at (http:\/\/\S+)/i,
onSpawn: killOnTeardown(t, (c) => (seen = c)),
what: "probe",
timeoutMs: 10_000,
pollMs: 25,
});
assert.equal(seen, child, "onSpawn got the child that was returned");
assert.equal(match[1], "http://127.0.0.1:9999");
});
test("scans stdout as well as stderr", async (t) => {
const { match } = await startAnnouncedChild({
command: process.execPath,
args: ["-e", 'console.log("ready on 4242")'],
pattern: /ready on (\d+)/,
onSpawn: killOnTeardown(t),
what: "probe",
timeoutMs: 10_000,
pollMs: 25,
});
assert.equal(match[1], "4242");
});
test("sees an announcement that lands inside the final polling interval", async (t) => {
// The off-by-one this guards: a fixed attempt count checks the output and
// then sleeps, so nothing ever observes the last interval — an announcement
// at 29.9s of a 30s budget reads as a timeout.
//
// Setting pollMs === timeoutMs makes that deterministic rather than a race:
// the attempt-counted loop gets exactly one check, at t≈0 before the child
// has even finished starting, and then throws. The deadline loop re-reads
// after the wait, so *any* announcement before the deadline is seen. The
// child announces immediately, ~2s inside the window on either side, so a
// loaded runner cannot flip the outcome. (Verified: this fails against the
// attempt-counted loop and passes against the deadline loop.)
const { match } = await startAnnouncedChild({
command: process.execPath,
args: ["-e", 'console.error("listening at http://127.0.0.1:8080")'],
pattern: /listening at (http:\/\/\S+)/i,
onSpawn: killOnTeardown(t),
what: "probe",
timeoutMs: 2000,
pollMs: 2000,
});
assert.equal(match[1], "http://127.0.0.1:8080");
});
test("a non-announcing child is still reachable and killable after the timeout", async (t) => {
// The regression: a child that is alive but never announces. The throw must
// not be the only thing that happens — the caller must already hold the
// handle, or `process.exit(1)` leaves this running.
let published = null;
await assert.rejects(
startAnnouncedChild({
// Sleeps well past the budget and prints nothing matching.
command: process.execPath,
args: ["-e", "setTimeout(() => {}, 60000)"],
pattern: /never going to match/,
onSpawn: killOnTeardown(t, (c) => (published = c)),
what: "probe",
timeoutMs: 300,
pollMs: 25,
}),
/probe did not start within/,
);
assert.ok(published, "the child was published before the readiness wait");
assert.equal(
published.exitCode,
null,
"and it is still alive, as in the bug",
);
published.kill("SIGKILL");
await waitForExit(published);
assert.ok(
published.exitCode !== null || published.signalCode !== null,
"teardown could actually stop it",
);
});
test("reports a child that exits before announcing, with its output", async (t) => {
let published = null;
await assert.rejects(
startAnnouncedChild({
command: process.execPath,
args: ["-e", 'console.error("boom: bad config"); process.exit(3)'],
pattern: /never going to match/,
onSpawn: killOnTeardown(t, (c) => (published = c)),
what: "probe",
timeoutMs: 10_000,
pollMs: 25,
}),
(err) =>
/probe exited early/.test(err.message) &&
/boom: bad config/.test(err.message),
);
assert.ok(published, "published even on the early-exit path");
});
test("reports a spawn failure rather than throwing it uncaught", async (t) => {
await assert.rejects(
startAnnouncedChild({
command: "definitely-not-an-executable-2000",
args: [],
pattern: /never/,
onSpawn: killOnTeardown(t),
what: "probe",
timeoutMs: 10_000,
pollMs: 25,
}),
/could not spawn the probe:/,
);
});

67
scripts/smoke-web-app.mjs
View file

@ -46,13 +46,13 @@
* demand if missing, as in smoke:cli.
*/
import { spawn, spawnSync } from "node:child_process";
import { spawnSync } from "node:child_process";
import { existsSync } from "node:fs";
import { createRequire } from "node:module";
import { setTimeout as delay } from "node:timers/promises";
import { join, resolve } from "node:path";
import { startProdWebServer } from "./lib/prod-web-server.mjs";
import { stopChild } from "./lib/child-cleanup.mjs";
import { startAnnouncedChild } from "./lib/announced-child.mjs";
import { resolveNodeBin } from "./lib/resolve-node-bin.mjs";
const repoRoot = resolve(import.meta.dirname, "..");
@ -164,45 +164,30 @@ function ensureTestServer() {
/**
* Spawn the MCP App test server and wait for it to announce its URL.
*
* Both stdio channels are piped and scanned: server-composable.ts announces
* readiness with `console.error`, so watching stdout alone never matches and
* this times out with an empty diagnostic. Piping both also keeps the child's
* noise out of the smoke's own output while still making it available in the
* failure message.
* The spawn/readiness ownership lives in `lib/announced-child.mjs` so the
* failure path is testable: this smoke's happy path always gets the
* announcement, so nothing here could prove that a child which stays alive
* through the readiness timeout is still reachable by `shutdown()` — the case
* that used to orphan a live server holding its port (#2000). `onSpawn`
* publishes the child to `mcpServer` before the wait, so every throw path is
* covered by teardown.
*
* The announced URL is authoritative rather than the config's port:
* createTestServerHttp resolves through findAvailablePort(), which walks upward
* when the configured value is taken.
*/
async function startMcpServer() {
const child = spawn(
process.execPath,
[composableServer, "--config", appConfig],
{ cwd: repoRoot, stdio: ["ignore", "pipe", "pipe"] },
);
let out = "";
child.stdout.on("data", (d) => (out += d));
child.stderr.on("data", (d) => (out += d));
let exited = false;
let spawnError = null;
// A spawn failure (e.g. an unbuilt/renamed entry) emits `error`, NOT `exit` —
// and with no `error` listener Node throws it uncaught, replacing this smoke's
// diagnostic with a raw stack. `close` is listened to alongside `exit` for the
// same reason: it fires in cases `exit` does not, so a child that dies without
// an exit event can't leave the poll below spinning for the full 30s.
child.on("error", (err) => (spawnError = err));
child.on("exit", () => (exited = true));
child.on("close", () => (exited = true));
for (let attempt = 0; attempt < 120; attempt++) {
// Take the port the server actually bound, not the one we asked for.
const announced = out.match(/listening at (http:\/\/\S+)/i);
if (announced) return { child, url: announced[1] };
if (spawnError) {
throw new Error(
`could not spawn the MCP test server (${composableServer}): ${spawnError.message}`,
);
}
if (exited) throw new Error(`MCP test server exited early:\n${out}`);
await delay(250);
}
throw new Error(`MCP test server did not start within 30s:\n${out}`);
const { match } = await startAnnouncedChild({
command: process.execPath,
args: [composableServer, "--config", appConfig],
cwd: repoRoot,
pattern: /listening at (http:\/\/\S+)/i,
onSpawn: (child) => {
mcpServer = child;
},
what: `MCP test server (${composableServer})`,
});
return { url: match[1] };
}
/** base64url(JSON) — the appArgs encoding the deep link expects. */
@ -247,7 +232,9 @@ try {
}
ensureTestServer();
({ child: mcpServer, url: mcpUrl } = await startMcpServer());
// `startMcpServer` publishes the child to `mcpServer` itself, so teardown
// reaches it even when this throws before returning.
({ url: mcpUrl } = await startMcpServer());
await server.waitForReady();
browser = await loadChromium();
const page = await browser.newPage();