diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index ec31e4fa..d4c90e4a 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -55,6 +55,13 @@ jobs: node clients/launcher/build/index.js --cli --help node clients/launcher/build/index.js --tui --help + - name: Smoke test prod web launcher (end-to-end) + # Starts `mcp-inspector --web` against the built clients/web/dist (from + # the "Run Build" step) and asserts GET / serves the SPA (HTTP 200) with + # the injected auth token — the prod web path the --help smoke can't + # reach (#1486). + run: npm run smoke:web + - name: Cache Playwright browsers uses: actions/cache@v4 with: diff --git a/AGENTS.md b/AGENTS.md index d09dcc5f..0d81de80 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -134,7 +134,8 @@ All work should be driven by items on the project board. ### Lint-fixed, Formatted code - ALWAYS do `npm run format` before committing — it auto-fixes any Prettier issues. `validate` runs `format:check` (the non-fixing variant) and will fail in CI on any unformatted file, so always run the auto-fixer first rather than letting `format:check` catch it. -- ALWAYS do `npm run validate` before pushing any changes — from the repo root it mirrors what CI enforces across every client: web `format:check` + `lint` + `build` + `test:coverage` (`validate:web`), then the CLI and TUI test suites (`test:cli` / `test:tui`), then a build of CLI + TUI + launcher and the launcher `--help` smoke (`validate:launcher`). The sub-scripts (`validate:web`, `validate:launcher`, `smoke:launcher`) can be run individually when iterating on one client. Storybook is the only CI step left out (see below). +- ALWAYS do `npm run validate` before pushing any changes — from the repo root it mirrors what CI enforces across every client: web `format:check` + `lint` + `build` + `test:coverage` (`validate:web`), then the CLI and TUI test suites (`test:cli` / `test:tui`), then a build of CLI + TUI + launcher, the launcher `--help` smoke (`smoke:launcher`), and the prod web end-to-end smoke (`smoke:web`) — together `validate:launcher`. The sub-scripts (`validate:web`, `validate:launcher`, `smoke:launcher`, `smoke:web`) can be run individually when iterating on one client. Storybook is the only CI step left out (see 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`. - Also run `npm run test:storybook` before pushing — it executes every story's `play` function in headless Chromium via `@vitest/browser-playwright` (~10s). CI runs this as a separate step after the unit/lint/build checks; failures block merge. It is kept out of `validate` because it needs the Playwright browser binary and is much slower than the unit suite. ### Typescript instructions diff --git a/clients/launcher/README.md b/clients/launcher/README.md index 70d2e1ac..490139d0 100644 --- a/clients/launcher/README.md +++ b/clients/launcher/README.md @@ -26,6 +26,22 @@ Rules: `--catalog` and `--config` are mutually exclusive; neither combines with an ad-hoc target or `--header`; `--header` requires an ad-hoc HTTP/SSE server and is applied to that connection (it is no longer a warn-only no-op). +### Production web build (`--web`, no `--dev`) + +Prod `--web` serves static assets from `clients/web/dist/`, which only exists +after a build. In the published package `dist/` always ships. In a fresh dev +checkout it is absent, so the runner **builds it on demand** the first time you +launch (`vite build` via `npm run build:client`, run in `clients/web`) instead +of serving a broken page (#1486). If that build can't run — e.g. dev +dependencies are missing — the launcher exits with an actionable error telling +you to run `npm run build` (from `clients/web`) or relaunch with `--dev` to use +the Vite dev server. `--dev` never needs `dist/`; it runs Vite directly. + +CI and `npm run validate` (via `validate:launcher`) exercise this prod path +end-to-end with `npm run smoke:web` (`scripts/smoke-web.mjs`): it starts +`mcp-inspector --web` against the built `dist/` and asserts `GET /` returns the +SPA (HTTP 200) with the injected `__INSPECTOR_API_TOKEN__`. + ## Publishing The root `@modelcontextprotocol/inspector` package ships as one fat tarball: `npm run build` at the repo root builds all clients, then `prepack` runs before `npm publish`. Runtime dependencies are declared on the root `package.json`; client builds bundle `@inspector/core` and externalize npm packages resolved from the root install. diff --git a/clients/web/package.json b/clients/web/package.json index f81ff2d9..5a5c98ad 100644 --- a/clients/web/package.json +++ b/clients/web/package.json @@ -13,6 +13,7 @@ "scripts": { "dev": "vite", "build": "tsc -b && vite build && npm run build:runner", + "build:client": "vite build", "build:runner": "tsup --config tsup.runner.config.ts", "lint": "eslint .", "format": "prettier --write src", diff --git a/clients/web/server/ensure-web-build.ts b/clients/web/server/ensure-web-build.ts new file mode 100644 index 00000000..e485d4ab --- /dev/null +++ b/clients/web/server/ensure-web-build.ts @@ -0,0 +1,76 @@ +/** + * Ensure the production web assets exist before the Hono server tries to serve + * them (#1486). + * + * `mcp-inspector --web` (prod, no `--dev`) serves static files from + * `clients/web/dist/`, which only exists after a build. In the published package + * `dist/` always ships, so this is a no-op there. In a fresh dev checkout the + * directory is absent and the prod path would otherwise serve a broken page + * (`ENOENT` on `index.html`). When the build is missing we build it once, on + * demand; if that build can't run (e.g. dev dependencies are absent, as in a + * stripped published install) we throw an actionable error instead of letting + * the server come up and serve a broken page. + */ + +import { existsSync } from "node:fs"; +import { join } from "node:path"; +import { spawnSync } from "node:child_process"; + +/** Seams for testing — defaults hit the real filesystem / child process. */ +export interface EnsureWebBuildDeps { + exists?: (path: string) => boolean; + /** Build the web assets in `webRoot`; returns the child exit code (null if it never ran). */ + build?: (webRoot: string, log: (message: string) => void) => number | null; + log?: (message: string) => void; +} + +function runViteBuild( + webRoot: string, + log: (message: string) => void, +): number | null { + const result = spawnSync("npm", ["run", "build:client"], { + cwd: webRoot, + stdio: "inherit", + // npm resolves to npm.cmd on Windows, which needs a shell to be spawnable. + shell: process.platform === "win32", + }); + // A spawn failure (e.g. `npm` not on PATH → ENOENT) leaves `status` null and + // the child produces no output of its own, so surface the cause before the + // caller falls back to the generic actionable error. + if (result.error) { + log(`Web build failed to start: ${result.error.message}`); + } + return result.status; +} + +/** + * Build `clients/web/dist` on demand when it is missing so `--web` always has + * something to serve. `webRoot` is the web client root (where `package.json` and + * the `build:client` script live); `distRoot` is the static-asset directory the + * Hono server serves from. Throws an actionable error if the build can't be + * produced — the caller surfaces the message and exits non-zero. + */ +export function ensureWebBuild( + webRoot: string, + distRoot: string, + deps: EnsureWebBuildDeps = {}, +): void { + const exists = deps.exists ?? existsSync; + const build = deps.build ?? runViteBuild; + const log = deps.log ?? ((message: string) => console.log(message)); + + const indexHtml = join(distRoot, "index.html"); + if (exists(indexHtml)) return; + + log( + "No production web build found at clients/web/dist. Building the web UI now (first run only; this can take a minute)...", + ); + + const status = build(webRoot, log); + + if (status !== 0 || !exists(indexHtml)) { + throw new Error( + "Could not build the web UI automatically. Build it manually with `npm run build` (run from clients/web), or launch with `--dev` to use the Vite dev server.", + ); + } +} diff --git a/clients/web/server/run-web.ts b/clients/web/server/run-web.ts index d5725374..e98ed9b8 100644 --- a/clients/web/server/run-web.ts +++ b/clients/web/server/run-web.ts @@ -21,6 +21,7 @@ import { import { buildWebServerConfig } from "./web-server-config.js"; import { startViteDevServer } from "./start-vite-dev-server.js"; import { startHonoServer } from "./server.js"; +import { ensureWebBuild } from "./ensure-web-build.js"; const __dirname = dirname(fileURLToPath(import.meta.url)); @@ -237,8 +238,10 @@ export async function runWeb(argv: string[]): Promise { writable, initialServers, }); + const webRoot = join(__dirname, ".."); + const distRoot = join(webRoot, "dist"); if (!isDev) { - webConfig.staticRoot = join(__dirname, "..", "dist"); + webConfig.staticRoot = distRoot; } console.log( @@ -253,6 +256,9 @@ export async function runWeb(argv: string[]): Promise { if (isDev) { handle = await startViteDevServer(webConfig); } else { + // Build clients/web/dist on demand if it's missing so prod `--web` never + // serves a broken page (#1486); throws an actionable error if it can't. + ensureWebBuild(webRoot, distRoot); handle = await startHonoServer(webConfig); } } catch (err) { diff --git a/clients/web/src/test/integration/server/ensure-web-build.test.ts b/clients/web/src/test/integration/server/ensure-web-build.test.ts new file mode 100644 index 00000000..97d389d4 --- /dev/null +++ b/clients/web/src/test/integration/server/ensure-web-build.test.ts @@ -0,0 +1,148 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { join } from "node:path"; + +const { existsSync, spawnSync } = vi.hoisted(() => ({ + existsSync: vi.fn(), + spawnSync: vi.fn(), +})); + +vi.mock("node:fs", () => ({ existsSync })); +vi.mock("node:child_process", () => ({ spawnSync })); + +import { ensureWebBuild } from "../../../../server/ensure-web-build.js"; + +const WEB_ROOT = "/repo/clients/web"; +const DIST_ROOT = join(WEB_ROOT, "dist"); +const INDEX_HTML = join(DIST_ROOT, "index.html"); + +describe("ensureWebBuild", () => { + it("is a no-op when the build already exists", () => { + const exists = vi.fn().mockReturnValue(true); + const build = vi.fn(); + const log = vi.fn(); + + ensureWebBuild(WEB_ROOT, DIST_ROOT, { exists, build, log }); + + expect(exists).toHaveBeenCalledWith(INDEX_HTML); + expect(build).not.toHaveBeenCalled(); + expect(log).not.toHaveBeenCalled(); + }); + + it("builds on demand when the build is missing, then proceeds", () => { + // Missing before the build, present after. + const exists = vi.fn().mockReturnValueOnce(false).mockReturnValueOnce(true); + const build = vi.fn().mockReturnValue(0); + const log = vi.fn(); + + expect(() => + ensureWebBuild(WEB_ROOT, DIST_ROOT, { exists, build, log }), + ).not.toThrow(); + + expect(build).toHaveBeenCalledWith(WEB_ROOT, expect.any(Function)); + expect(log).toHaveBeenCalledTimes(1); + expect(log.mock.calls[0]?.[0]).toContain("No production web build found"); + }); + + it("throws an actionable error when the build exits non-zero", () => { + const exists = vi.fn().mockReturnValue(false); + const build = vi.fn().mockReturnValue(1); + + expect(() => + ensureWebBuild(WEB_ROOT, DIST_ROOT, { exists, build, log: vi.fn() }), + ).toThrow(/Could not build the web UI automatically/); + }); + + it("throws when the build can't even run (null exit status)", () => { + const exists = vi.fn().mockReturnValue(false); + const build = vi.fn().mockReturnValue(null); + + expect(() => + ensureWebBuild(WEB_ROOT, DIST_ROOT, { exists, build, log: vi.fn() }), + ).toThrow(/`--dev`/); + }); + + it("throws when the build succeeds but still produces no index.html", () => { + // Exit 0 but the asset is still absent (e.g. a partial/aborted build). + const exists = vi.fn().mockReturnValue(false); + const build = vi.fn().mockReturnValue(0); + + expect(() => + ensureWebBuild(WEB_ROOT, DIST_ROOT, { exists, build, log: vi.fn() }), + ).toThrow(/Could not build the web UI/); + expect(build).toHaveBeenCalledTimes(1); + }); + + // Default seams (real `existsSync` / `spawnSync` / `console.log`) so the + // production code path — not just the injected one — is exercised. + describe("with default dependencies", () => { + beforeEach(() => { + existsSync.mockReset(); + spawnSync.mockReset(); + }); + + it("is a no-op when index.html already exists (no spawn)", () => { + existsSync.mockReturnValue(true); + + ensureWebBuild(WEB_ROOT, DIST_ROOT); + + expect(existsSync).toHaveBeenCalledWith(INDEX_HTML); + expect(spawnSync).not.toHaveBeenCalled(); + }); + + it("spawns `npm run build:client` in the web root when missing", () => { + existsSync.mockReturnValueOnce(false).mockReturnValueOnce(true); + spawnSync.mockReturnValue({ status: 0 }); + const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + + try { + ensureWebBuild(WEB_ROOT, DIST_ROOT); + } finally { + logSpy.mockRestore(); + } + + expect(spawnSync).toHaveBeenCalledWith( + "npm", + ["run", "build:client"], + expect.objectContaining({ cwd: WEB_ROOT, stdio: "inherit" }), + ); + }); + + it("throws when the spawned build exits non-zero", () => { + existsSync.mockReturnValue(false); + spawnSync.mockReturnValue({ status: 1 }); + const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + + try { + expect(() => ensureWebBuild(WEB_ROOT, DIST_ROOT)).toThrow( + /Could not build the web UI automatically/, + ); + } finally { + logSpy.mockRestore(); + } + }); + + it("surfaces the spawn error when the build can't start (ENOENT)", () => { + existsSync.mockReturnValue(false); + spawnSync.mockReturnValue({ + status: null, + error: new Error("spawn npm ENOENT"), + }); + const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + + try { + expect(() => ensureWebBuild(WEB_ROOT, DIST_ROOT)).toThrow( + /Could not build the web UI/, + ); + expect( + logSpy.mock.calls.some((c) => + String(c[0]).includes( + "Web build failed to start: spawn npm ENOENT", + ), + ), + ).toBe(true); + } finally { + logSpy.mockRestore(); + } + }); + }); +}); diff --git a/clients/web/src/test/integration/server/run-web.test.ts b/clients/web/src/test/integration/server/run-web.test.ts index 029f1224..28b29104 100644 --- a/clients/web/src/test/integration/server/run-web.test.ts +++ b/clients/web/src/test/integration/server/run-web.test.ts @@ -4,14 +4,18 @@ import { existsSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -const { mockClose, startViteDevServer, startHonoServer } = vi.hoisted(() => { - const mockClose = vi.fn().mockResolvedValue(undefined); - return { - mockClose, - startViteDevServer: vi.fn().mockResolvedValue({ close: mockClose }), - startHonoServer: vi.fn().mockResolvedValue({ close: mockClose }), - }; -}); +const { mockClose, startViteDevServer, startHonoServer, ensureWebBuild } = + vi.hoisted(() => { + const mockClose = vi.fn().mockResolvedValue(undefined); + return { + mockClose, + startViteDevServer: vi.fn().mockResolvedValue({ close: mockClose }), + startHonoServer: vi.fn().mockResolvedValue({ close: mockClose }), + // No-op by default so prod tests don't shell out to a real build; the + // build-on-demand contract is covered in ensure-web-build.test.ts. + ensureWebBuild: vi.fn(), + }; + }); vi.mock("../../../../server/start-vite-dev-server.js", () => ({ startViteDevServer, @@ -21,6 +25,10 @@ vi.mock("../../../../server/server.js", () => ({ startHonoServer, })); +vi.mock("../../../../server/ensure-web-build.js", () => ({ + ensureWebBuild, +})); + import * as nodeConfig from "../../../../../../core/mcp/node/config.js"; import { runWeb } from "../../../../server/run-web.js"; @@ -39,6 +47,8 @@ describe("runWeb", () => { beforeEach(() => { startViteDevServer.mockClear(); startHonoServer.mockClear(); + ensureWebBuild.mockClear(); + ensureWebBuild.mockReset(); mockClose.mockClear(); signalHandlers.clear(); exitCode = undefined; @@ -126,6 +136,32 @@ describe("runWeb", () => { ); }); + it("ensures the web build before starting Hono in production", async () => { + void runWeb(["node", "run-web"]); + await expectServerStarted(startHonoServer); + + expect(ensureWebBuild).toHaveBeenCalledTimes(1); + // (webRoot, distRoot) — both end at the web client root / its dist dir. + const [webRoot, distRoot] = ensureWebBuild.mock.calls[0] as [ + string, + string, + ]; + expect(distRoot).toMatch(/dist$/); + expect(distRoot.startsWith(webRoot)).toBe(true); + }); + + it("exits with an actionable error when the web build can't be produced", async () => { + ensureWebBuild.mockImplementationOnce(() => { + throw new Error("Could not build the web UI automatically."); + }); + + await expect(runWeb(["node", "run-web"])).rejects.toThrow("process.exit:1"); + expect( + errorLines.some((l) => l.includes("Could not build the web UI")), + ).toBe(true); + expect(startHonoServer).not.toHaveBeenCalled(); + }); + it("starts Vite when --dev is set", async () => { void runWeb(["node", "run-web", "--dev"]); await expectServerStarted(startViteDevServer); @@ -134,6 +170,8 @@ describe("runWeb", () => { expect.objectContaining({ initialMcpConfig: null, writable: true }), ); expect(logLines.some((l) => l.includes("development mode"))).toBe(true); + // --dev uses Vite directly; no static build is required or attempted. + expect(ensureWebBuild).not.toHaveBeenCalled(); }); // --- ad-hoc launch → read-only in-memory session ------------------------ diff --git a/package.json b/package.json index b973fafa..c7f488c9 100644 --- a/package.json +++ b/package.json @@ -39,8 +39,9 @@ "test:tui": "cd clients/tui && npm run test", "validate": "npm run validate:web && npm run test:cli && npm run test:tui && npm run validate:launcher", "validate:web": "cd clients/web && npm run validate", - "validate:launcher": "npm run build:cli && npm run build:tui && npm run build:launcher && npm run smoke:launcher", + "validate:launcher": "npm run build:cli && npm run build:tui && npm run build:launcher && npm run smoke:launcher && npm run smoke:web", "smoke:launcher": "node clients/launcher/build/index.js --help && node clients/launcher/build/index.js --cli --help && node clients/launcher/build/index.js --tui --help", + "smoke:web": "node scripts/smoke-web.mjs", "test": "cd ./clients/web/ && npm run test && cd ../cli && npm run test && cd ../tui && npm run test", "test:coverage": "cd ./clients/web/ && npm run test:coverage", "test:storybook": "cd ./clients/web/ && npm run test:storybook", diff --git a/scripts/smoke-web.mjs b/scripts/smoke-web.mjs new file mode 100644 index 00000000..ca0ee37c --- /dev/null +++ b/scripts/smoke-web.mjs @@ -0,0 +1,100 @@ +#!/usr/bin/env node +/** + * End-to-end smoke test for the prod web launcher path (#1486). + * + * `npm run smoke:launcher` only checks `--help` for each mode; it never starts + * the prod web server, so the static-asset / injected-token path went unverified + * in CI. This script launches `mcp-inspector --web` (prod, no `--dev`) against + * the built `clients/web/dist`, waits for it to listen, and asserts that `/` + * returns HTTP 200 with the injected auth-token global. It exits non-zero + * (failing CI / `npm run validate`) on any failure, then shuts the server down. + * + * Expects `clients/web/dist` and `clients/launcher/build` to be built first — + * the validate / CI ordering guarantees this, so the readiness wait below is + * sized for an already-built `dist`. (If `dist` happened to be missing, the + * launcher's build-on-demand path would build it on startup, which a cold + * `vite build` could push past that wait — not a scenario this script targets.) + */ + +import { spawn } from "node:child_process"; +import { setTimeout as delay } from "node:timers/promises"; + +const HOST = "127.0.0.1"; +const PORT = process.env.SMOKE_WEB_PORT ?? "6299"; +const TOKEN = "smoke-web-token"; +// Mirrors INSPECTOR_API_TOKEN_GLOBAL in core/mcp/remote/constants.ts; kept as a +// literal because this plain .mjs script can't import the TS source. +const TOKEN_GLOBAL = "__INSPECTOR_API_TOKEN__"; +const BASE_URL = `http://${HOST}:${PORT}`; + +const child = spawn( + process.execPath, + ["clients/launcher/build/index.js", "--web"], + { + env: { + ...process.env, + CLIENT_PORT: PORT, + HOST, + MCP_INSPECTOR_API_TOKEN: TOKEN, + // Don't pop a browser in CI. + MCP_AUTO_OPEN_ENABLED: "false", + }, + stdio: ["ignore", "inherit", "inherit"], + }, +); + +let exited = false; +let exitCode = null; +child.on("exit", (code) => { + exited = true; + exitCode = code; +}); + +function shutdown() { + if (!exited) child.kill("SIGTERM"); +} + +function fail(message) { + console.error(`smoke:web FAILED — ${message}`); + shutdown(); + process.exit(1); +} + +async function fetchRoot() { + for (let attempt = 0; attempt < 120; attempt++) { + if (exited) { + throw new Error( + `launcher exited (code ${exitCode}) before serving — see output above`, + ); + } + try { + return await fetch(`${BASE_URL}/`); + } catch { + await delay(500); + } + } + throw new Error("server did not start within 60s"); +} + +try { + const res = await fetchRoot(); + if (res.status !== 200) { + fail(`GET / returned HTTP ${res.status}, expected 200`); + } + const body = await res.text(); + if (!body.includes(TOKEN_GLOBAL)) { + fail( + `served HTML is missing the ${TOKEN_GLOBAL} global (token not injected)`, + ); + } + if (!body.includes(TOKEN)) { + fail("served HTML is missing the injected auth-token value"); + } + console.log( + `smoke:web OK — GET / => 200 with injected ${TOKEN_GLOBAL} at ${BASE_URL}`, + ); + shutdown(); + process.exit(0); +} catch (err) { + fail(err instanceof Error ? err.message : String(err)); +}