diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index de5ff9f4..6f0c77cb 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -105,6 +105,7 @@ Both exist and do different jobs. Theme files (`src/theme/.ts`) custo - `npm run format` before committing; **`npm run ci` before pushing** (`validate` → `coverage` → `verify:build-gate` → `verify:bundle-externals` → `smoke` → `smoke:web:firefox` → Storybook; the Firefox step is local-only and not mirrored in GitHub CI). `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`; `pack:verify`'s `--web` child sets its own `MCP_CATALOG_PATH` for the same reason (#2003 — its App deep link persists a server row). Anything that boots the web backend and then *navigates* it needs that isolation, not just the scripts named `smoke:*`. A new smoke spawning its own server, or teardown that removes a work dir without first awaiting `stopChild` (the #1801 race — `child-cleanup.mjs` exports both halves and both are required), should be flagged. +- **A smoke must assert the thing survives, not that it started.** `smoke:tui` runs the Ink TUI under a pseudoterminal (`scripts/lib/pty.mjs`) and requires it to still be running `SMOKE_TUI_SURVIVE_MS` after its first frame (`scripts/lib/render-smoke.mjs`) — because it previously settled OK on first paint while the TUI was exiting 1 ~40ms later with `Raw mode is not supported`, and won that race on every machine (#2147). Flag anything that collapses the survival wait back into "resolve on the marker", and any new smoke whose only assertion is that a process printed something once. A smoke walks only its own happy path, so the branch that matters — started, then died — belongs in a `scripts/lib/*.test.mjs` driven against a stub (as with `announced-child.mjs` #2000 and `ensure-test-servers.mjs` #2111), not in the smoke. - **The headless web smokes are engine-parameterized, not Chromium-only.** `smoke:web:browser` / `smoke:web:app` / `smoke:web:elicit` take their engine from `SMOKE_BROWSER` (chromium — the default — firefox, webkit). **GitHub CI runs Chromium only; Firefox runs in the local pre-push gate** via `smoke:web:firefox` (#2086 — a CI job was trialled and removed for never disagreeing with Chromium across a dozen runs, and moved into `npm run ci` instead). They exist because the MCP Apps sandbox is built out of the primitives that diverge between engines: `srcdoc` CSP inheritance, nested sandboxed iframes, `Permissions-Policy`, cross-frame `postMessage`. Nothing else covers that — `sandbox-csp.test.ts` asserts a policy *string*, and no Storybook story reaches the sandbox at all. Flag a new browser-driven script that launches Playwright itself instead of going through `scripts/lib/headless-browser.mjs`, an unrecognized-engine path that falls back to Chromium rather than failing (it would claim coverage that never ran), or a launch-failure message naming the wrong engine. `pack:verify` is pinned to Chromium on purpose — it is a packaging check. - **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/.github/workflows/main.yml b/.github/workflows/main.yml index a7191880..4ab26676 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -1,4 +1,4 @@ -name: Run install, format, lint, build, and test on every push +name: CI on: push: @@ -43,27 +43,15 @@ jobs: # self-validation: format:check + lint + typecheck + build + test (no # coverage instrumentation — fast). This also builds every client bundle # (web dist, cli/tui/launcher) that the smokes below need. The heavier - # per-file coverage gate runs in its own step below. Unit tests run in - # both steps (fast here, instrumented under coverage); the double unit - # run is an accepted trade-off for keeping this fast build step intact. - # A future optimization could split coverage into per-client parallel + # per-file coverage gate runs in the parallel `coverage` job below + # (#2159), which consumes nothing this job produces — every client's + # `test:coverage` builds whatever it needs itself. Unit tests run in + # both jobs (fast here, instrumented there); now that the two run in + # parallel that duplication costs no wall clock at all. + # A future optimization could split `coverage` into per-client parallel # jobs, but that's a larger restructure and deliberately out of scope. run: npm run validate - - name: Enforce per-file coverage gate (≥90% on all four dimensions) - # CI-ENFORCED coverage gate (#1550): runs `npm run coverage`, which - # chains every client's `test:coverage` (v8-instrumented) and fails the - # job if ANY file drops below 90% on lines, statements, functions, or - # branches. This is the whole point of the step — a PR that regresses - # coverage below the threshold now blocks merge instead of relying on a - # contributor remembering to run the gate locally. - # - # This also covers the web integration project: web's `test:coverage` - # runs `--project=unit --project=integration --coverage`, so the former - # standalone `Run web integration tests` step was removed to avoid - # running the integration suite twice. - run: npm run coverage - - name: Verify the browser-externalized-builtin build gate (#1769) # Runs a real `vite build` with a Node built-in forced into the browser # graph and asserts the build FAILS via the #1769 gate. The unit tests @@ -115,6 +103,49 @@ jobs: working-directory: ./clients/web run: npm run test:storybook + # The per-file coverage gate, in its own job so it runs in PARALLEL with + # `build` rather than serially after it (#2159). The two together were 84% of + # a ~17m wall clock; split, the workflow finishes in roughly the length of + # `build` alone. + # + # This is safe because `coverage` consumes nothing `build` produces: every + # client's `test:coverage` is self-sufficient (web and cli build the test + # servers — and cli its own bin — themselves; tui and launcher run from + # source). The only consumers of `clients/*/build` are the smokes, which stay + # in `build` alongside the `validate` that produces them. + # + # Do NOT "optimize" this back into one job by backgrounding the two commands: + # that puts two vitest fleets on one 4-core runner, which is already known to + # time tests out at the 5s default. Separate jobs get separate runners. + coverage: + runs-on: ubuntu-latest + + steps: + - name: Checkout code + uses: actions/checkout@v7 + + - name: Setup Node.js + uses: actions/setup-node@v7 + with: + node-version: '22.x' + cache: 'npm' + + - name: Install dependencies (root + all clients) + run: npm install + + - name: Enforce per-file coverage gate (≥90% on all four dimensions) + # CI-ENFORCED coverage gate (#1550): runs `npm run coverage`, which + # chains every client's `test:coverage` (v8-instrumented) and fails the + # job if ANY file drops below 90% on lines, statements, functions, or + # branches. This is the whole point of the job — a PR that regresses + # coverage below the threshold blocks merge instead of relying on a + # contributor remembering to run the gate locally. + # + # This also covers the web integration project: web's `test:coverage` + # runs `--project=unit --project=integration --coverage`, so there is no + # separate integration step anywhere in this workflow. + run: npm run coverage + # Publish the single `@modelcontextprotocol/inspector` package to npm on a # published GitHub release. v2 is not an npm workspace, so there is no # `publish-all` / `--workspaces` (v1) — just one `npm publish`, whose `prepack` @@ -127,7 +158,7 @@ jobs: runs-on: ubuntu-latest if: github.event_name == 'release' environment: release - needs: build + needs: [build, coverage] # Serialize publishes so two releases cut in quick succession can't run # overlapping `npm publish`es. Never cancel an in-flight publish. concurrency: @@ -220,7 +251,7 @@ jobs: runs-on: ubuntu-latest if: github.event_name == 'release' environment: release - needs: build + needs: [build, coverage] permissions: contents: read packages: write diff --git a/AGENTS.md b/AGENTS.md index 256ec709..568a228b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -931,7 +931,7 @@ The ⚠️ option-deletion hazard, the snapshot rule, and the recovery recipe ab - Run the root tooling's own tests with `npm run test:scripts` from the root — `node --test "scripts/**/*.test.mjs"`, node's built-in runner (the root has no vitest harness by design). A new `scripts/*.mjs` helper with pure logic gets a sibling `*.test.mjs`; keep the filename `*.test.mjs`, since `node --test` silently **skips** a file its glob misses and still exits 0 (`verify:typecheck-coverage` guards against exactly that). - **The test tiers, shallowest first:** unit (`test`, per client) → web integration (`test:integration`, real transports/servers) → out-of-process (`clients/cli/__tests__/e2e.test.ts`, spawns the built binary) → smokes through the built launcher (`npm run smoke`) → Storybook play functions (`test:storybook`) → the published-tarball check (`npm run pack:verify`, local/release only — needs network). `validate` runs the per-client `test` scripts — so web **unit** plus cli's out-of-process `e2e.test.ts` (it's part of cli's `test`), but **not** web's integration project, which runs inside the `coverage` gate. Everything from `smoke` rightward is `npm run ci` only, and is described under [Mandatory pre-push gate](#mandatory-pre-push-gate). - The repo root has no aggregate `test` script — each client self-validates, so run `npm run validate` from the root (all clients, fast) or `cd clients/ && npm run validate` (one client). Each client still exposes its own `test` / `test:coverage` for quick iteration. -- **`validate` is fast: it runs `test`, not `test:coverage`.** The coverage gate (slower — adds v8 instrumentation, and for web the integration project) is a **separate** top-level `npm run coverage` (and per-client `coverage:web` / `coverage:cli` / `coverage:tui` / `coverage:launcher`, each delegating to that client's `test:coverage`). Run `npm run coverage` when you want to reproduce the gate locally before pushing. **CI runs `coverage`** on every push (#1550): the per-file ≥90 gate is CI-enforced, so a PR that drops any file below 90 on lines/statements/functions/branches fails the job. CI runs `validate` (fast) for format/lint/build/unit tests, then `coverage` for the instrumented gate. Because web's `test:coverage` already runs the integration project, CI has no separate `test:integration` step — the integration paths are exercised inside the coverage gate. +- **`validate` is fast: it runs `test`, not `test:coverage`.** The coverage gate (slower — adds v8 instrumentation, and for web the integration project) is a **separate** top-level `npm run coverage` (and per-client `coverage:web` / `coverage:cli` / `coverage:tui` / `coverage:launcher`, each delegating to that client's `test:coverage`). Run `npm run coverage` when you want to reproduce the gate locally before pushing. **CI runs `coverage`** on every push (#1550): the per-file ≥90 gate is CI-enforced, so a PR that drops any file below 90 on lines/statements/functions/branches fails the job. It runs in its **own job, in parallel with `build`** (#2159) — `coverage` consumes nothing `validate` produces, since each client's `test:coverage` builds whatever it needs itself, and running the two serially was 84% of a ~17m wall clock. Both publish jobs therefore gate on `needs: [build, coverage]`; leaving either off would let a release publish with the coverage gate red. Because web's `test:coverage` already runs the integration project, CI has no separate `test:integration` step — the integration paths are exercised inside the coverage gate. - Each client's `test:coverage` enforces a **uniform per-file gate of ≥ 90 on all four dimensions** — lines, statements, functions, and branches — across `clients/web`, `clients/cli`, `clients/tui`, and `clients/launcher` (CI enforces this gate). This is the result of a codebase-wide audit: the branch floor was first lifted 50 → 70 for web (#1271), then the whole gate raised to 90 with real tests added for every outlier. Genuinely-unreachable branches are **not** waved through by lowering the gate — they are annotated at the source with a justified `/* v8 ignore … -- */` comment. Acceptable reasons are happy-dom-inherent paths (Mantine portal mount points, `useMediaQuery` fallbacks, `typeof window` SSR guards), React StrictMode effect-replay blocks, and provably-dead defensive guards (e.g. a `?? fallback` for a value the types guarantee non-null, or a `Select.onChange` receiving a value outside the allowed list). New code must clear 90 on every dimension; reach for a justified `v8 ignore` only when a branch is genuinely impossible to exercise. The web coverage `include` (in `clients/web/vite.config.ts`) covers the shared `core/` runtime consumed by the browser — `core/mcp`, `core/react`, `core/auth`, `core/storage`, `core/logging`, `core/node`, **`core/json`, and `core/client`** (the last two folded in by #1689). When adding a `core/json/*` or `core/client/*` module, its tests live under `clients/web/src/test/core/…` and are gated the same ≥90 way. - The **same per-file gate** is enforced for the CLI and TUI (#1484), not just web: - **CLI** (`clients/cli`): tests run **in-process** by importing `runCli()` (see `__tests__/helpers/cli-runner.ts`) so `clients/cli/src` is measured under v8 instrumentation. A thin out-of-process layer (`__tests__/e2e.test.ts` + `scripts/smoke-cli.mjs`) still spawns the built binary for the shebang/`process.exit` paths; `src/index.ts` (binary bootstrap) is the only coverage exclusion. `commander` uses `.exitOverride()` so a parse error throws instead of tearing down the test worker. @@ -963,7 +963,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`, as is `smoke:web:firefox` (the same browser-driven smokes again under Firefox, #2086 — CI does not run that step). 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` is NOT part of `validate`** — it is included in `npm run ci`, as is `smoke:web:firefox` (the same browser-driven smokes again under Firefox, #2086 — CI does not run that step). 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's `build` job runs `validate`, 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), then the Storybook play-function tests. The `coverage` gate (which also covers the web integration project) runs in a **parallel job** rather than in that chain (#2159), so GitHub CI is the same set of gates as `npm run ci` but no longer in the same order. - `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. @@ -973,7 +973,13 @@ The ⚠️ option-deletion hazard, the snapshot rule, and the recovery recipe ab - `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. -- `smoke:cli` (`scripts/smoke-cli.mjs`) drives `mcp-inspector --cli` through the built launcher against the bundled stdio test server via a temp `--catalog`: it asserts `tools/list` returns the server's tools (real connect over stdio), the default writable catalog is seeded empty on first run, a missing read-only `--config` errors without seeding, and `--catalog` + `--config` is rejected. `smoke:tui` (`scripts/smoke-tui.mjs`) launches `mcp-inspector --tui --catalog ` and asserts the Ink app renders its first frame (the "MCP Servers" panel) within a timeout, then SIGTERMs it — a shallow boot/render check, not full interaction. **`smoke:tui` is local-only: it self-skips when `process.env.CI` is set**, because the Ink TUI needs a real TTY (raw mode) that headless CI lacks — so run it (via `npm run smoke`) on your own machine before pushing. Both build `test-servers/build` on demand if it's missing. +- `smoke:cli` (`scripts/smoke-cli.mjs`) drives `mcp-inspector --cli` through the built launcher against the bundled stdio test server via a temp `--catalog`: it asserts `tools/list` returns the server's tools (real connect over stdio), the default writable catalog is seeded empty on first run, a missing read-only `--config` errors without seeding, and `--catalog` + `--config` is rejected. `smoke:tui` (`scripts/smoke-tui.mjs`) launches `mcp-inspector --tui --catalog ` **under a pseudoterminal**, asserts the Ink app renders its first frame (the "MCP Servers" panel) within a timeout, then waits `SMOKE_TUI_SURVIVE_MS` (default 2s) and asserts it is **still running** before SIGTERMing it — a shallow boot/render/survival check, not full interaction. Both rebuild `test-servers/build` on **every run** — once per process, whether or not it already exists (#2111): presence is not freshness, and a smoke driving a stale fixture reports a product failure rather than a staleness one. + + **The survival half is the assertion, and it is the whole point (#2147).** This smoke used to settle OK the instant the marker appeared and never look at the child again. It was spawning the TUI with `stdio: ["ignore", …]`, i.e. stdin on `/dev/null`; Ink mounts `useInput`, `useInput` needs raw mode, and raw mode is a property of the **file descriptor** — so the TUI painted one frame and died ~40ms later with `Raw mode is not supported on the current process.stdin`. The smoke won that ~40ms race and printed OK. What it asserted was *first paint*, while its own header claimed "boots and renders without crashing", and it had never once verified a running TUI **on any machine** — `stdio: ["ignore", …]` makes the child's stdin `/dev/null` whether or not the developer's own terminal is a TTY. The race also cut both ways: an `exit` processed first failed the run, which reads as flake rather than as the standing defect it was. + + Two pieces, and the order matters. **`scripts/lib/pty.mjs`** gives the child a real terminal via `script(1)` — no dependency, and the flavors are not interchangeable (BSD takes argv, util-linux takes one shell string plus `-e`, busybox that string without `-e`), so the invocation is built and unit-tested rather than written inline; a platform with no `script(1)` (Windows) **skips** rather than running without one, since that is a guaranteed failure and not a weaker check. **`scripts/lib/render-smoke.mjs`** is the fix: it requires the child to outlive its first frame. Without that, a future harness change reintroduces the same false green and nothing notices — which is why it lives in a module `test:scripts` drives against a stub that paints the marker and immediately exits (the old harness passes that stub; this one must not), rather than in a smoke that only ever walks its own happy path. Do **not** collapse the survival wait back into "resolve on the marker" to save two seconds. + + **`smoke:tui` is still local-only: it self-skips when `process.env.CI` is set** — now by decision rather than by capability. The PTY removes the technical blocker the skip used to cite; whether it joins GitHub CI is a separate call, and #2146 deliberately keeps the other local-only smokes out. So run it (via `npm run smoke`) on your own machine before pushing: a local-only gate is exactly where a false green is least likely to be caught by anything else. - Storybook play-function tests (`clients/web` `test:storybook`) run in headless Chromium via `@vitest/browser-playwright` (~10s). They are part of `npm run ci` (which installs Playwright chromium first); kept out of `validate` because they need the browser binary and are slower than the unit suite. ### The web smokes are engine-parameterized; Firefox is in the pre-push gate, not CI (#2086) diff --git a/README.md b/README.md index b2e05c2d..3e49e844 100644 --- a/README.md +++ b/README.md @@ -486,11 +486,11 @@ Each client self-validates from its own folder; the root scripts chain them. The | ----------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | `npm run validate` | Runs the three durable guards first — `verify:format-coverage` (every tracked source file is format-gated), `verify:typecheck-coverage` (every one lands in a tsconfig project), `verify:dep-lockstep` (no dependency reaching one `tsc` program from two installs skews across them) — then `test:scripts` (the guards' own parser unit tests), then `validate:core` (the shared `core/` `format:check` + `lint` gate), then per client: `format:check` + `lint` + **`typecheck`** (cli/tui/launcher; web typechecks via `tsc -b` inside its `build`) + `build` + fast unit tests. The quick inner-loop check. | | `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-browser 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. All three take their engine from `SMOKE_BROWSER` — see [Supported browsers](#supported-browsers). | +| `npm run smoke` | End-to-end smokes through the built launcher (`--help` dispatch + prod cli/tui/web — the TUI one runs under a **pseudoterminal** and asserts the Ink app is still running two seconds after its first frame, not merely that it painted one, [#2147](https://github.com/modelcontextprotocol/inspector/issues/2147)), plus three headless-browser 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. All three take their engine from `SMOKE_BROWSER` — see [Supported browsers](#supported-browsers). | | `npm run verify:build-gate` | Runs a real `vite build` with a Node built-in forced into the browser graph and asserts the build **fails** via the #1769 gate (which turns Vite's browser-externalization warning into a hard error). Guards against the warning phrasing drifting in a Vite bump and silently disabling the gate. Part of `npm run ci`. | | `npm run verify:bundle-externals` | Guards the must-not-bundle invariant (#2067): for each tsup-bundled client it reads the **built** `build/` output and fails if any package that must stay external was inlined anyway. Candidates are the union of the client's own `external` array **and the root manifest's `dependencies`** — the latter because #2067 was a *missing* `external` entry, which a self-referential check would have passed. Detection is via esbuild's `// ` module banners, so it covers both shapes — a separate `-HASH.js` chunk (what a dynamically `import()`ed CommonJS package produces) and a statically-imported package folded straight into `index.js`, which emits no chunk at all. A build with no banners fails as such rather than passing clean, so enabling `minify` cannot silently retire the check. `undici` was declared only in the root and `clients/cli` manifests, and tsup auto-externalizes only what the _nearest_ manifest declares, so the web and TUI bundles inlined 1.05MB of it — and CommonJS inlined into an ESM bundle throws `Dynamic require of "assert" is not supported` on first use, from a specifier no user-side install can satisfy. Reads the output rather than the config because those two disagreed for four releases. Part of `npm run ci`. | | `npm run verify:format-coverage` | Parses the `format:check` globs out of every `package.json` (only those reachable from `validate`), enumerates all tracked source files, and **fails** listing any not covered by a glob — the durable guard for the "every first-party source file is format-gated" invariant (#1792). Runs first in `validate`. | -| `npm run test:scripts` | Table-driven unit tests (`node --test`) for the guard's own pure parsers (`scripts/lib/npm-scripts.mjs`, `scripts/lib/tsc-program.mjs` + the exported helpers of `verify-typecheck-coverage.mjs` and `verify-dep-lockstep.mjs`), one case per rule they encode, plus two suites over shared `scripts/lib` helpers that no smoke can check itself: `resolve-node-bin.test.mjs` — the cross-platform bin resolver (#1939), pinned against the real `bin`/`exports` shapes of the packages the scripts actually spawn — and `announced-child.test.mjs` — the spawn/readiness ownership helper (#2000), which drives real `node -e` children to prove a child that never announces is still published to the caller before the timeout throws, and so is reachable by teardown rather than orphaned. Two more do the same: `mcp-app-flow.test.mjs` covers the shared MCP Apps flow (#2003) — the deep link's two CSRF gates and `appArgs` encoding, plus `driveAppFlow`'s failure branches against a stand-in page, all of which are dead code from the happy-path smokes' point of view and would otherwise surface only as opaque timeouts; and `ensure-test-servers.test.mjs` pins the [#2111](https://github.com/modelcontextprotocol/inspector/issues/2111) invariant — that `test-servers/build` is rebuilt **even when it already exists** — which no smoke can assert about itself, since one driving a stale fixture reports a product failure rather than a staleness one. 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. Four more do the same: `mcp-app-flow.test.mjs` covers the shared MCP Apps flow (#2003) — the deep link's two CSRF gates and `appArgs` encoding, plus `driveAppFlow`'s failure branches against a stand-in page, all of which are dead code from the happy-path smokes' point of view and would otherwise surface only as opaque timeouts; `render-smoke.test.mjs` and `pty.test.mjs` cover the TUI boot harness ([#2147](https://github.com/modelcontextprotocol/inspector/issues/2147)) — the former driving real `node -e` stubs to prove that a child which paints the marker and *then* dies is a **failure**, which the old harness reported as OK and which no fixed TUI can reproduce; the latter pinning the three mutually-incompatible `script(1)` invocations, where a wrong guess stops the pseudoterminal from being allocated at all; and `ensure-test-servers.test.mjs` pins the [#2111](https://github.com/modelcontextprotocol/inspector/issues/2111) invariant — that `test-servers/build` is rebuilt **even when it already exists** — which no smoke can assert about itself, since one driving a stale fixture reports a product failure rather than a staleness one. 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` → `verify:bundle-externals` → `smoke` → `smoke:web:firefox` → Storybook. A strict superset of GitHub CI — `smoke:web:firefox` (and `smoke:tui`) run only here. | @@ -547,7 +547,7 @@ The `smoke:*` scripts run against the in-repo build tree, which is **not** the p ### Cutting a release -Publishing is automated by two release-gated jobs in [`.github/workflows/main.yml`](.github/workflows/main.yml) (`github.event_name == 'release'`, both `needs: build`): +Publishing is automated by two release-gated jobs in [`.github/workflows/main.yml`](.github/workflows/main.yml) (`github.event_name == 'release'`, both `needs: [build, coverage]` — so a release cannot publish with either the build job or the coverage gate red): - **`publish`** — the npm package. Runs `npm run pack:verify` as the pre-publish gate, asserts the release tag matches the root `package.json` version, then `npm publish --access public --provenance` — a single `npm publish` (v2 is not an npm workspace, so there is no v1-style `publish-all`/`--workspaces`), with a signed provenance attestation via GitHub OIDC (`id-token: write`, `environment: release`, `NPM_TOKEN`). - **`publish-github-container-registry`** — the container image (see [Docker](#docker)). diff --git a/clients/launcher/README.md b/clients/launcher/README.md index e2fc94f4..cea9e258 100644 --- a/clients/launcher/README.md +++ b/clients/launcher/README.md @@ -87,11 +87,21 @@ through the built launcher artifact (beyond the `--help` checks in resolution paths (default-catalog seed-on-missing, read-only `--config` error-without-seed, `--catalog`/`--config` conflict). - `npm run smoke:tui` (`scripts/smoke-tui.mjs`) — launches - `mcp-inspector --tui --catalog ` and asserts the Ink app renders its - first frame within a timeout, then shuts it down (a shallow boot/render + `mcp-inspector --tui --catalog ` **under a pseudoterminal**, asserts the + Ink app renders its first frame within a timeout, then waits and asserts it is + **still running** before shutting it down (a shallow boot/render/survival check, not full interaction). -Both build `test-servers/build` on demand if it is missing. + The second half is the assertion (#2147). Spawned with its stdin on + `/dev/null`, Ink cannot enter raw mode for `useInput`, so the TUI painted one + frame and exited 1 about 40ms later — and this smoke, which settled OK on the + first frame, won that race and reported success on every machine. It is + local-only (self-skips under `CI`), which is precisely where a false green + goes unnoticed. + +Both rebuild `test-servers/build` on **every run** — once per process, whether +or not it already exists (#2111). Presence is not freshness: a smoke driving a +stale fixture reports a product failure rather than a staleness one. ## Development diff --git a/scripts/lib/pty.mjs b/scripts/lib/pty.mjs new file mode 100644 index 00000000..d311ba68 --- /dev/null +++ b/scripts/lib/pty.mjs @@ -0,0 +1,180 @@ +/** + * Wrap a command so it runs attached to a pseudoterminal (#2147). + * + * `smoke:tui` spawns the Ink TUI with `stdio: ["ignore", …]`, which makes the + * child's stdin `/dev/null`. Ink mounts `useInput`, `useInput` needs raw mode, + * and raw mode is a property of the *file descriptor* — so the child threw + * "Raw mode is not supported on the current process.stdin" and exited 1 about + * 40ms after painting its first frame, on every machine, TTY or not. A parent + * cannot assert raw mode on a child's behalf; the fd has to be a terminal. + * + * `script(1)` allocates one, with no dependency to install. Measured here on + * darwin: without it the TUI paints at ~400ms and is dead at ~440ms; with it + * the same TUI is still running at 6s and never logs the raw-mode error. + * + * The invocation differs by flavor, which is why this is a module rather than + * a string literal at the call site: + * + * BSD (macOS) script -q /dev/null argv, no shell + * util-linux script -qec "" /dev/null ONE shell string + * busybox script -qc "" /dev/null as above, no -e + * + * The `-c` flavors take a single command *string* that the child shell parses, + * so every word has to be quoted on the way in — an unquoted temp path with a + * space would otherwise split into two arguments. + * + * `-e` (util-linux) makes `script` exit with the child's status rather than its + * own. busybox has no such flag, so under it a crashed child reports 0. That is + * survivable and deliberately not worked around: the smoke's assertion is the + * child's `exit` *event*, not its code — the code only sharpens the diagnostic. + */ + +import { spawnSync } from "node:child_process"; + +/** + * Platforms whose `script` takes the BSD argv form (`script -q FILE cmd args…`). + * + * Deliberately just these two. darwin is verified empirically here; freebsd is + * where darwin's implementation comes from and shares its usage. **netbsd and + * openbsd are excluded on purpose** — the BSDs do not agree on this, NetBSD's + * `script` takes the command through `-c` — and nobody has run either here. A + * guessed invocation is worse than none: it fails at `script` startup, which + * this smoke would then report as a TUI crash. They fall through to `null` and + * the smoke skips, saying so. Adding a `-c` flavor for them is a small change + * for whoever can actually verify it on the box. + */ +const BSD_PLATFORMS = new Set(["darwin", "freebsd"]); + +/** + * Quote one argument for a POSIX shell command string. + * + * Single quotes are literal for everything except a single quote itself, which + * has to leave the quoted run, emit an escaped quote, and re-enter it. + * + * @param {string} arg + * @returns {string} + */ +export function shellQuote(arg) { + return `'${String(arg).replaceAll("'", `'\\''`)}'`; +} + +/** + * Decide which `script` flavor to build for. + * + * The version output is consulted first because it is the only *evidence*: + * "linux" does not imply util-linux (Alpine ships busybox, whose `script` + * rejects `-e`). Platform is the fallback for the case where `script --version` + * tells us nothing, which is itself informative — BSD `script` has no + * `--version` and fails the probe. + * + * @param {object} opts + * @param {string} opts.platform `process.platform`. + * @param {string} [opts.versionOutput] Combined stdout+stderr of `script --version`. + * @returns {"bsd" | "util-linux" | "busybox" | null} null when unsupported. + */ +export function scriptFlavorFor({ platform, versionOutput = "" }) { + if (/util-linux/i.test(versionOutput)) return "util-linux"; + if (/busybox/i.test(versionOutput)) return "busybox"; + if (BSD_PLATFORMS.has(platform)) return "bsd"; + // Linux with an unidentifiable `script` is overwhelmingly util-linux; guess + // it rather than skip the smoke. A wrong guess fails loudly with `script`'s + // own usage error in the captured output, which is diagnosable — silently + // skipping is the outcome this issue exists to stop rewarding. + if (platform === "linux") return "util-linux"; + // win32 (no `script(1)` at all), and any platform whose invocation nobody has + // verified — aix, sunos, netbsd, openbsd. The caller skips loudly rather than + // guessing. `node-pty` is the portable answer if this ever has to run there. + return null; +} + +/** + * Build the PTY-wrapped spawn arguments for a command. + * + * @param {object} opts + * @param {string} opts.command + * @param {string[]} [opts.args] + * @param {"bsd" | "util-linux" | "busybox"} opts.flavor + * @returns {{ command: string, args: string[] }} + */ +export function ptyCommand({ command, args = [], flavor }) { + if (flavor === "bsd") { + return { command: "script", args: ["-q", "/dev/null", command, ...args] }; + } + if (flavor === "util-linux" || flavor === "busybox") { + const line = [command, ...args].map(shellQuote).join(" "); + const flags = flavor === "util-linux" ? "-qec" : "-qc"; + return { command: "script", args: [flags, line, "/dev/null"] }; + } + throw new Error(`unknown script(1) flavor: ${flavor}`); +} + +/** + * Probe the local `script(1)`: is it there, and what does it say about itself? + * + * **`spawnSync` does not throw on ENOENT** — it *returns* `{ error }` with + * empty output. So a `try`/`catch` alone cannot tell "no `script` on this box" + * from "a `script` that printed nothing", and collapsing both to `""` is + * actively harmful: on linux the empty output falls through to the + * `util-linux` guess, and `smoke:tui` then hard-fails spawning a binary that + * does not exist, instead of taking the documented skip. Hence the `available` + * flag rather than a bare string. The `catch` stays for a runner that really + * does throw (an injected one, or a future spawn shape). + * + * A non-zero exit is NOT unavailability: BSD `script` has no `--version` and + * answers `illegal option -- -` plus its usage on stderr — which is exactly the + * evidence `scriptFlavorFor` reads. Only `error` means "could not run it". + * + * @param {(cmd: string, args: string[]) => { stdout?: string, stderr?: string, error?: Error }} [runner] + * Injected for tests; defaults to a real `spawnSync`. + * @returns {{ available: boolean, output: string }} + */ +export function probeScriptVersion( + runner = (cmd, args) => + spawnSync(cmd, args, { encoding: "utf8", timeout: 5000 }), +) { + try { + const r = runner("script", ["--version"]); + // `error` covers ENOENT, EACCES and the timeout; a missing result at all + // means the runner told us nothing, which is not evidence of a working one. + if (!r || r.error) return { available: false, output: "" }; + return { available: true, output: `${r.stdout ?? ""}${r.stderr ?? ""}` }; + } catch { + return { available: false, output: "" }; + } +} + +/** + * Resolve a PTY wrapper for this machine, or say why there isn't one. + * + * The two ways it can be unavailable are worth telling apart in the skip + * message — "you have no `script(1)`" and "nobody has verified this platform's + * invocation" send the reader somewhere different — so the failure carries a + * `reason` rather than being a bare `null` the caller has to narrate. + * + * @param {object} [opts] + * @param {string} [opts.platform] + * @param {() => { available: boolean, output: string }} [opts.probe] + * @returns {{ ok: true, flavor: string, wrap: (spec: { command: string, args?: string[] }) => { command: string, args: string[] } } + * | { ok: false, reason: string }} + */ +export function resolvePtyWrapper({ + platform = process.platform, + probe = probeScriptVersion, +} = {}) { + const { available, output } = probe(); + if (!available) { + return { ok: false, reason: "no `script(1)` on PATH" }; + } + const flavor = scriptFlavorFor({ platform, versionOutput: output }); + if (!flavor) { + return { + ok: false, + reason: `no verified \`script(1)\` invocation for ${platform}`, + }; + } + return { + ok: true, + flavor, + wrap: ({ command, args = [] }) => ptyCommand({ command, args, flavor }), + }; +} diff --git a/scripts/lib/pty.test.mjs b/scripts/lib/pty.test.mjs new file mode 100644 index 00000000..ceb1fbcb --- /dev/null +++ b/scripts/lib/pty.test.mjs @@ -0,0 +1,237 @@ +import { strict as assert } from "node:assert"; +import test from "node:test"; + +import { + ptyCommand, + probeScriptVersion, + resolvePtyWrapper, + scriptFlavorFor, + shellQuote, +} from "./pty.mjs"; + +// The flavors are not interchangeable: BSD `script` takes argv after the +// typescript file, util-linux takes ONE shell string before it, and busybox +// takes that string without `-e`. Getting it wrong is not a subtle failure — +// `script` refuses to start, and the smoke reports it as a TUI crash. +test("scriptFlavorFor prefers the version probe over the platform", () => { + assert.equal( + scriptFlavorFor({ + platform: "linux", + versionOutput: "script from util-linux 2.39.3", + }), + "util-linux", + ); + assert.equal( + scriptFlavorFor({ + platform: "linux", + versionOutput: "BusyBox v1.36.1 (2024-01-01) multi-call binary.", + }), + "busybox", + ); + // Alpine is the reason the probe exists at all: "linux" does not imply + // util-linux, and busybox's `script` rejects `-e`. + assert.notEqual( + scriptFlavorFor({ platform: "linux", versionOutput: "BusyBox v1.36.1" }), + "util-linux", + ); +}); + +test("scriptFlavorFor falls back to the platform when the probe says nothing", () => { + // BSD `script` has no --version, so failing the probe is itself the signal. + assert.equal(scriptFlavorFor({ platform: "darwin" }), "bsd"); + assert.equal(scriptFlavorFor({ platform: "freebsd" }), "bsd"); + // NOT every BSD: NetBSD takes the command through `-c`, and nobody has run + // either it or OpenBSD here. Guessing builds an invocation that fails at + // `script` startup, which this smoke would then report as a TUI crash. + assert.equal(scriptFlavorFor({ platform: "netbsd" }), null); + assert.equal(scriptFlavorFor({ platform: "openbsd" }), null); + // An unidentifiable `script` on linux is overwhelmingly util-linux. Guess it: + // a wrong guess fails loudly with `script`'s own usage error in the captured + // output, whereas skipping is the silence this issue exists to stop. + assert.equal(scriptFlavorFor({ platform: "linux" }), "util-linux"); +}); + +test("scriptFlavorFor returns null where there is no verified invocation", () => { + assert.equal(scriptFlavorFor({ platform: "win32" }), null); + assert.equal( + scriptFlavorFor({ platform: "win32", versionOutput: "not a thing" }), + null, + ); + // Unsupported is the default, not a win32 special case. + assert.equal(scriptFlavorFor({ platform: "aix" }), null); + assert.equal(scriptFlavorFor({ platform: "sunos" }), null); +}); + +test("ptyCommand builds the BSD argv form", () => { + assert.deepEqual( + ptyCommand({ + command: "/bin/node", + args: ["app.js", "--x"], + flavor: "bsd", + }), + { + command: "script", + args: ["-q", "/dev/null", "/bin/node", "app.js", "--x"], + }, + ); +}); + +test("ptyCommand builds the util-linux shell-string form with -e", () => { + // -e is what makes `script` exit with the child's status instead of its own. + assert.deepEqual( + ptyCommand({ + command: "/bin/node", + args: ["app.js"], + flavor: "util-linux", + }), + { + command: "script", + args: ["-qec", "'/bin/node' 'app.js'", "/dev/null"], + }, + ); +}); + +test("ptyCommand omits -e for busybox, which has no such flag", () => { + const { args } = ptyCommand({ + command: "node", + args: ["app.js"], + flavor: "busybox", + }); + assert.equal(args[0], "-qc"); + assert.deepEqual(args, ["-qc", "'node' 'app.js'", "/dev/null"]); +}); + +test("ptyCommand quotes arguments for the shell-string flavors", () => { + // The smoke passes an mkdtemp path; a space in it would otherwise split into + // two arguments inside the shell `script -c` runs. + const { args } = ptyCommand({ + command: "/usr/local/my node/bin/node", + args: ["/tmp/a dir/catalog.json", "it's-fine"], + flavor: "util-linux", + }); + assert.equal( + args[1], + `'/usr/local/my node/bin/node' '/tmp/a dir/catalog.json' 'it'\\''s-fine'`, + ); +}); + +test("shellQuote survives an embedded single quote", () => { + assert.equal(shellQuote("it's"), `'it'\\''s'`); + assert.equal(shellQuote("plain"), "'plain'"); +}); + +test("probeScriptVersion combines both streams and never throws", () => { + assert.deepEqual( + probeScriptVersion(() => ({ stdout: "out ", stderr: "err" })), + { available: true, output: "out err" }, + ); + // A non-zero exit is NOT unavailability. BSD `script` has no `--version` and + // answers with `illegal option -- -` plus its usage on stderr — which is the + // very evidence the flavor fallback reads, so it must survive the probe. + assert.deepEqual( + probeScriptVersion(() => ({ + status: 1, + stdout: "", + stderr: "script: illegal option -- -\nusage: script [-aeFkpqr] ...", + })), + { + available: true, + output: "script: illegal option -- -\nusage: script [-aeFkpqr] ...", + }, + ); + // A runner that throws outright (injected, or a future spawn shape). + assert.deepEqual( + probeScriptVersion(() => { + throw new Error("ENOENT"); + }), + { available: false, output: "" }, + ); + // Told nothing at all is not evidence of a working `script`. + assert.deepEqual( + probeScriptVersion(() => undefined), + { + available: false, + output: "", + }, + ); +}); + +// `spawnSync` REPORTS ENOENT in `result.error` — it does not throw — so a +// try/catch alone reads a missing binary as "ran, printed nothing". On linux +// that empty output falls through to the util-linux guess, and `smoke:tui` +// hard-fails spawning a binary that does not exist instead of skipping. +test("probeScriptVersion treats result.error as unavailable, not as empty output", () => { + const enoent = Object.assign(new Error("spawnSync script ENOENT"), { + code: "ENOENT", + }); + assert.deepEqual( + probeScriptVersion(() => ({ error: enoent, stdout: "", stderr: "" })), + { available: false, output: "" }, + ); + // The timeout shape reports the same way. + assert.equal( + probeScriptVersion(() => ({ error: new Error("ETIMEDOUT") })).available, + false, + ); +}); + +test("resolvePtyWrapper reports a missing script(1) rather than guessing", () => { + // linux would otherwise be classified util-linux off the empty output. + const r = resolvePtyWrapper({ + platform: "linux", + probe: () => ({ available: false, output: "" }), + }); + assert.equal(r.ok, false); + assert.match(r.reason, /on PATH/); +}); + +test("resolvePtyWrapper wraps with the resolved flavor, or reports why not", () => { + const wrapper = resolvePtyWrapper({ + platform: "linux", + probe: () => ({ available: true, output: "script from util-linux 2.39.3" }), + }); + assert.equal(wrapper.ok, true); + assert.equal(wrapper.flavor, "util-linux"); + assert.deepEqual(wrapper.wrap({ command: "node", args: ["a.js"] }), { + command: "script", + args: ["-qec", "'node' 'a.js'", "/dev/null"], + }); + // `args` is optional at the call site. + assert.deepEqual(wrapper.wrap({ command: "node" }).args[1], "'node'"); + + // A present `script` on a platform whose invocation nobody verified is a + // DIFFERENT failure from a missing one, and the reason must say which. + const unsupported = resolvePtyWrapper({ + platform: "win32", + probe: () => ({ available: true, output: "" }), + }); + assert.equal(unsupported.ok, false); + assert.match(unsupported.reason, /no verified .* invocation for win32/); +}); + +// The platforms this repo claims a verified `script(1)` invocation for. Any +// other — win32, aix, sunos, netbsd, openbsd — resolves to null BY DESIGN and +// makes `smoke:tui` skip, so asserting "supported unless Windows" would fail +// `test:scripts` on a machine where the smoke is behaving exactly as intended. +const SUPPORTED_PLATFORMS = new Set(["darwin", "freebsd", "linux"]); + +test("resolvePtyWrapper resolves on the platforms we claim to support", () => { + // Not a tautology: it runs the real probe against the real machine, so a + // `script` that has vanished from PATH surfaces here rather than as an opaque + // smoke failure. + const wrapper = resolvePtyWrapper(); + if (SUPPORTED_PLATFORMS.has(process.platform)) { + assert.equal( + wrapper.ok, + true, + `no PTY wrapper on ${process.platform}: ${wrapper.reason ?? ""}`, + ); + assert.equal(wrapper.wrap({ command: "node" }).command, "script"); + } else { + assert.equal( + wrapper.ok, + false, + `${process.platform} is not in SUPPORTED_PLATFORMS but resolved a wrapper`, + ); + } +}); diff --git a/scripts/lib/render-smoke.mjs b/scripts/lib/render-smoke.mjs new file mode 100644 index 00000000..bff51b24 --- /dev/null +++ b/scripts/lib/render-smoke.mjs @@ -0,0 +1,332 @@ +/** + * Spawn a terminal app, wait for it to paint a marker, and then wait again to + * see whether it is still running (#2147). + * + * Extracted from `scripts/smoke-tui.mjs` for the same reason + * `announced-child.mjs` was extracted from `smoke-web-app.mjs` (#2000): a smoke + * script only ever exercises its own happy path, so the branch that matters + * most here — a child that paints the marker and *then* dies — is unreachable + * from the real TUI once it is fixed. Driving this from a test against a stub + * that paints and immediately exits 1 is what keeps the assertion honest. + * + * The defect it encodes against: `smoke:tui` used to resolve OK the instant the + * marker appeared and never looked at the child again. The TUI was exiting 1 + * about 40ms later, every run, on every machine, and the smoke won that race + * and printed OK. So it asserted "painted once", while its own header claimed + * "boots and renders without crashing". Worse, the race cut both ways: had the + * `exit` event been processed first, the same run would have failed, which + * reads as flake rather than as the standing defect it was. + * + * Hence `surviveMs`. First paint is not survival, and nothing shorter than an + * explicit "still alive N ms later" check can tell the two apart. + */ + +import { spawn } from "node:child_process"; + +/** + * Fallback durations, in ms. These are defaults, NOT minimums: `normalizeMs` + * accepts any finite positive value, and the tests deliberately pass shorter + * ones. What it rejects is the shape a bad env var produces, not smallness. + */ +export const DEFAULTS = Object.freeze({ + timeoutMs: 15_000, + surviveMs: 2_000, + exitGraceMs: 5_000, + drainMs: 500, +}); + +/** + * Coerce a caller-supplied duration to a usable one, mirroring + * `child-cleanup.mjs`'s `normalizeGraceMs`. + * + * Call sites read these from the environment, and `Number()` there has two + * holes that `??` does not cover: `Number("")` is `0` and `Number("typo")` is + * `NaN`. `setTimeout` treats both as "fire immediately" — which for `surviveMs` + * means the survival check resolves before the child could possibly have died, + * silently restoring the exact first-paint false green this module exists to + * prevent, on a run that still prints OK. Falling back is therefore not + * defensive tidiness; it is the difference between a gate and a decoration. + * + * A consequence worth stating: `surviveMs: 0` is NOT honored as "skip the + * survival wait". There is no legitimate caller for that here, and reading it + * literally is indistinguishable from the typo. + * + * @param {unknown} value + * @param {number} fallback + * @returns {number} + */ +export function normalizeMs(value, fallback) { + return typeof value === "number" && Number.isFinite(value) && value > 0 + ? value + : fallback; +} + +/** + * Tail of a child's output, for quoting in a diagnostic. Slicing a terminal + * stream can land mid-CSI-sequence, so drop through the first newline rather + * than lead with an escape-code fragment that mangles the line after it. + * + * @param {string} output + * @param {number} [limit] + * @returns {string} + */ +export function outputTail(output, limit = 800) { + const tail = output.slice(-limit); + const nl = tail.indexOf("\n"); + return output.length > limit && nl !== -1 ? tail.slice(nl + 1) : tail; +} + +/** + * @param {object} opts + * @param {string} opts.command Executable to spawn. + * @param {string[]} [opts.args] Arguments for it. + * @param {object} [opts.spawnOptions] Merged into the `spawn` options + * (`stdio` is fixed — see below). + * @param {string} opts.marker Substring whose appearance in the + * child's output counts as first paint. + * @param {number} [opts.timeoutMs] Budget for first paint (default 15s; + * non-finite/non-positive falls back). + * @param {number} [opts.surviveMs] How long after first paint the child + * must stay alive (default 2s; + * non-finite/non-positive falls back — + * 0 is a typo, never "skip the check"). + * @param {number} [opts.exitGraceMs] SIGTERM → SIGKILL grace (default 5s; + * non-finite/non-positive falls back). + * @param {number} [opts.drainMs] Post-exit pipe-drain cap (default 500ms; + * non-finite/non-positive falls back). + * @param {(child: import("node:child_process").ChildProcess) => void} [opts.onSpawn] + * Called synchronously with the child, before any waiting — the caller's + * teardown handle on every failure path. + * @param {{ pattern: RegExp, reason: string }} [opts.forbidOutput] + * Text that must NOT appear in an otherwise-passing run. `smoke:tui` uses it + * for Ink's raw-mode error: survival already implies its absence *today*, + * because that throw is fatal — but if Ink ever degraded it to a warning, a + * TUI with no keyboard input would survive happily and pass. Checked here + * rather than at the call site so it runs against the fully drained output + * and is reachable from a test. Use a non-global RegExp: `test()` on a `/g` + * pattern advances `lastIndex` and alternates. + * @param {(message: string) => void} [opts.warn] + * @returns {Promise<{ code: 0 | 1, message: string, output: string }>} + * On the normal path, resolves only after the child's streams have `close`d — + * so `output` is complete and the caller may remove a directory the child was + * using without re-entering the #1801 ENOTEMPTY race. + * + * That is **bounded, not guaranteed**. Both drain deadlines below resolve + * *without* having observed `close`, after warning: the child may still hold + * the dir, and `output` may be truncated mid-line. Cleanup at the call site + * must therefore be best-effort (`removeSafe`, not `rmSync`) — the worst case + * is a warning and a leaked temp dir, never a red smoke. Do not write a + * caller that depends on the close having happened. + */ +export function runRenderSmoke({ + command, + args = [], + spawnOptions = {}, + marker, + timeoutMs: requestedTimeoutMs, + surviveMs: requestedSurviveMs, + exitGraceMs: requestedExitGraceMs, + drainMs: requestedDrainMs, + forbidOutput, + onSpawn = () => {}, + warn = (m) => console.warn(m), +}) { + // Normalized rather than defaulted, so an env-derived 0/NaN cannot reach a + // `setTimeout` — see `normalizeMs`. Done once, before anything reads them, + // so the interpolated durations in the messages below can't read `NaNms`. + const timeoutMs = normalizeMs(requestedTimeoutMs, DEFAULTS.timeoutMs); + const surviveMs = normalizeMs(requestedSurviveMs, DEFAULTS.surviveMs); + const exitGraceMs = normalizeMs(requestedExitGraceMs, DEFAULTS.exitGraceMs); + const drainMs = normalizeMs(requestedDrainMs, DEFAULTS.drainMs); + + return new Promise((resolve) => { + const child = spawn(command, args, { + ...spawnOptions, + // stdin is `ignore` (i.e. /dev/null) deliberately, even under a PTY: BSD + // `script` refuses to start when its own stdin is a pipe + // ("tcgetattr/ioctl: Operation not supported on socket"). It reads the + // immediate EOF and forwards one ^D into the pty before the app mounts, + // which is harmless — verified against the real TUI, which survives it. + stdio: ["ignore", "pipe", "pipe"], + }); + // Publish before waiting, so every path below leaves the caller holding a + // stoppable handle rather than an orphan (the #2000 rule). + onSpawn(child); + + let output = ""; + let settled = false; + let finished = false; + let childExited = false; + let childClosed = false; + let markerAt = null; + /** + * Both deadlines. Declared up front because `onData` and `done` each clear + * them from closures defined above the `setTimeout` calls that assign them. + * + * @type {ReturnType | undefined} + */ + let surviveTimer; + /** @type {ReturnType | undefined} */ + let renderTimer; + + const t0 = Date.now(); + const since = () => Date.now() - t0; + + const finish = (code, message) => { + if (finished) return; + finished = true; + let verdict = code; + let text = typeof message === "function" ? message() : message; + // Only ever turns a pass into a failure. A run that already failed keeps + // its own diagnostic: the crash reason is the more useful of the two, and + // forbidden output is usually a *symptom* of it rather than a separate + // finding. + if (verdict === 0 && forbidOutput?.pattern.test(output)) { + verdict = 1; + text = `${forbidOutput.reason}\n${outputTail(output)}`; + } + resolve({ code: verdict, message: text, output }); + }; + + // Settle the verdict, then wait for the child to actually close before + // resolving (#1801). The caller's work dir doubles as the app's HOME, so it + // is still being written to when we signal; removing it while those writes + // land fails with ENOTEMPTY. Waiting on `close` rather than `exit` also + // covers a *spawn failure*, which emits `error` + `close` and never `exit` + // — and guarantees the pipes are drained, which is what makes the thunked + // messages below complete rather than truncated at the interesting line. + // + // Deliberately not `lib/child-cleanup.mjs`'s `stopChild()`: that resolves + // on the FIRST of exit/close, whereas this wait must keep going for `close` + // after `exit`, on a shorter re-armed deadline, for the drain. + const done = (code, message) => { + if (settled) return; + settled = true; + clearTimeout(renderTimer); + clearTimeout(surviveTimer); + if (childClosed) { + finish(code, message); + return; + } + const forceKill = setTimeout(() => child.kill("SIGKILL"), exitGraceMs); + + let deadline; + const armDeadline = (ms, warning) => { + clearTimeout(deadline); + deadline = setTimeout(() => { + warn(`render-smoke — ${warning}`); + finish(code, message); + }, ms); + }; + armDeadline( + exitGraceMs * 2, + `child did not close its output streams within ${exitGraceMs * 2}ms; giving up on the drain`, + ); + + // Once the child itself is gone only the pipe drain remains, so the wait + // drops to `drainMs`. `close` is bounded by whoever holds the stdio + // pipes, which can outlive the direct child: any descendant that + // inherited them keeps it pending. + const drainOnly = () => { + clearTimeout(forceKill); + armDeadline( + drainMs, + `child exited but held its output streams open for ${drainMs}ms (a descendant may have inherited them); giving up on the drain`, + ); + }; + if (childExited) drainOnly(); + else child.once("exit", drainOnly); + + child.once("close", () => { + clearTimeout(forceKill); + clearTimeout(deadline); + finish(code, message); + }); + + // Only signal a child that is still running: on the crash and + // spawn-failure paths there is nothing left to signal, and we are here + // purely to wait out the remaining `close`. + if ( + child.exitCode === null && + child.signalCode === null && + !child.killed + ) { + child.kill("SIGTERM"); + } + }; + + const onData = (chunk) => { + output += chunk.toString(); + if (markerAt !== null || !output.includes(marker)) return; + markerAt = since(); + // Disarm the first-paint deadline. It is first-caller-wins with `done`, + // so leaving it armed means a paint landing near `timeoutMs` lets it fire + // *during* the survival window and settle the run as "did not render + // " — quoting an output tail that visibly contains the marker. + // First paint has happened; its deadline no longer has a question to ask. + clearTimeout(renderTimer); + // `exit` can beat the last of the child's buffered output, and `done` + // then keeps reading until `close` — so this can run *after* the verdict + // is settled. Recording `markerAt` is still right (the deferred message + // below reads it, and a marker that only surfaced in the drain was still + // painted). Arming a timer is not: `done` is a no-op once settled, so it + // could not change the result, but it holds the event loop open for the + // whole survival window — which under `node --test` is a suite that + // hangs on for two seconds per case for no reason. + if (settled) return; + // First paint is not the verdict — it only starts the clock on the + // question this smoke actually asks. + surviveTimer = setTimeout(() => { + done( + 0, + `rendered "${marker}" at ${markerAt}ms and was still running ${surviveMs}ms later`, + ); + }, surviveMs); + }; + + child.stdout.on("data", onData); + child.stderr.on("data", onData); + + // Registered before done()'s own one-shot listener, so this always runs + // first — done() can trust `childClosed` even when called from a handler. + child.on("close", () => { + childClosed = true; + }); + + child.on("exit", (code, signal) => { + childExited = true; + const exitAt = since(); + if (settled) return; + const how = signal ? `signal ${signal}` : `code ${code}`; + // Thunked: rendered at `finish()`, i.e. after `close`, so `markerAt` is + // whatever the FULL output showed — a marker still sitting in the pipe + // when `exit` fired is accounted for by the time this is read. + // + // Deliberately makes no claim about the order in which this process + // *observed* the two. `exit` and the last `data` event race, so a + // "N ms later" phrased off observation times would flip wording (and + // sign) run to run on the same child. The times are reported as the + // observations they are and the verdict does not rest on them. + done(1, () => + markerAt === null + ? `child exited (${how}) before rendering "${marker}"\n${outputTail(output)}` + : `child rendered "${marker}" and then exited (${how}) inside the ` + + `${surviveMs}ms survival window — it painted one frame, it did not run ` + + `(first paint seen at ${markerAt}ms, exit at ${exitAt}ms)` + + `\n${outputTail(output)}`, + ); + }); + + child.on("error", (err) => { + done(1, `failed to spawn ${command}: ${err.message}`); + }); + + renderTimer = setTimeout(() => { + done( + 1, + () => + `child did not render "${marker}" within ${timeoutMs}ms\n${outputTail(output)}`, + ); + }, timeoutMs); + }); +} diff --git a/scripts/lib/render-smoke.test.mjs b/scripts/lib/render-smoke.test.mjs new file mode 100644 index 00000000..a77b5468 --- /dev/null +++ b/scripts/lib/render-smoke.test.mjs @@ -0,0 +1,258 @@ +import { strict as assert } from "node:assert"; +import test from "node:test"; + +import { + DEFAULTS, + normalizeMs, + outputTail, + runRenderSmoke, +} from "./render-smoke.mjs"; + +const MARKER = "MCP Servers"; + +/** A stub terminal app, driven as a real child process rather than a spy. */ +function stub(body) { + return { command: process.execPath, args: ["-e", body] }; +} + +// `process.exitCode`, never `process.exit()`. Writing to a *pipe* is +// asynchronous, and `process.exit()` tears the process down without draining — +// so `console.log(MARKER); process.exit(1)` can lose the marker outright. That +// is not hypothetical: probing the drain race for this PR, a 4MB write followed +// by `process.exit()` arrived at the parent with the marker missing entirely. +// A stub that intermittently never paints would fail the very assertion meant +// to pin the paint-then-die path, and would fail it as "exited before +// rendering" — the neighbouring branch. Setting `exitCode` and letting the loop +// drain still exits immediately after the write; nothing holds it open. +const paintThenExit = (code) => + stub(`console.log(${JSON.stringify(MARKER)}); process.exitCode = ${code};`); +const paintThenLive = stub( + `console.log(${JSON.stringify(MARKER)}); setInterval(() => {}, 1000);`, +); + +const opts = { + marker: MARKER, + timeoutMs: 5000, + surviveMs: 400, + exitGraceMs: 500, + drainMs: 200, + warn: () => {}, +}; + +// The regression this module exists for. The old harness resolved OK on the +// marker and never looked again, so this exact stub — paint, then die — passed. +test("fails when the child paints the marker and then exits non-zero", async () => { + const r = await runRenderSmoke({ ...opts, ...paintThenExit(1) }); + assert.equal(r.code, 1, `expected failure, got: ${r.message}`); + assert.match(r.message, /code 1/); + assert.match(r.message, /it painted one frame, it did not run/); + // The diagnostic must not read as a crash *before* first paint — that was a + // different defect, and conflating them sends the reader to the wrong place. + assert.doesNotMatch(r.message, /before rendering/); +}); + +// `exit` races the child's last `data` event, so this stub can be observed +// either way round on the same machine. Both orders must produce the same +// verdict AND the same wording — a message phrased off observation order would +// flip run to run, which is how a stable failure gets written off as flake. +// Repeated because one pass proves nothing about a race. +test("the verdict does not depend on whether exit beats the final output", async () => { + for (let i = 0; i < 12; i++) { + const r = await runRenderSmoke({ ...opts, ...paintThenExit(1) }); + assert.equal(r.code, 1, `run ${i}: expected failure, got: ${r.message}`); + assert.match(r.message, /it painted one frame, it did not run/, `run ${i}`); + assert.doesNotMatch(r.message, /before rendering/, `run ${i}`); + } +}); + +// Exiting 0 right after painting is the same failure: the app did not run. It +// is called out separately because `script(1)` under busybox reports 0 for a +// crashed child, so the verdict cannot rest on the exit code. +test("fails when the child paints the marker and then exits zero", async () => { + const r = await runRenderSmoke({ ...opts, ...paintThenExit(0) }); + assert.equal(r.code, 1, `expected failure, got: ${r.message}`); + assert.match(r.message, /it painted one frame, it did not run/); +}); + +test("passes when the child paints the marker and keeps running", async () => { + const r = await runRenderSmoke({ ...opts, ...paintThenLive }); + assert.equal(r.code, 0, `expected pass, got: ${r.message}`); + assert.match(r.message, /still running 400ms later/); +}); + +// The first-paint deadline and the survival window overlap: `done` is +// first-caller-wins, so an armed render timer firing during the survival window +// settles the run as "did not render" — while quoting an output tail that +// visibly contains the marker. A slow-but-fine TUI would fail, intermittently, +// with a diagnostic pointing at the wrong thing entirely. +test("a paint landing just under the deadline is not failed by the render timer", async () => { + const paintAt = 250; + const r = await runRenderSmoke({ + ...opts, + ...stub( + `setTimeout(() => { console.log(${JSON.stringify(MARKER)}); ` + + `setInterval(() => {}, 1000); }, ${paintAt});`, + ), + // Deliberately tight: the render deadline expires while the survival window + // is still open, which is the whole shape of the bug. + timeoutMs: paintAt + 150, + surviveMs: 400, + }); + assert.equal(r.code, 0, `expected pass, got: ${r.message}`); + assert.doesNotMatch(r.message, /did not render/); +}); + +test("fails when the child exits before painting", async () => { + const r = await runRenderSmoke({ + ...opts, + // `exitCode`, not `exit()` — see `paintThenExit`. A forced exit can + // truncate the stderr write, making the `boom` assertion below flaky. + ...stub(`console.error("boom"); process.exitCode = 3;`), + }); + assert.equal(r.code, 1); + assert.match(r.message, /before rendering/); + assert.match(r.message, /code 3/); + // The tail is quoted after the drain, so the crash reason survives. + assert.match(r.message, /boom/); +}); + +test("fails when the child never paints, and kills it", async () => { + let spawned; + const r = await runRenderSmoke({ + ...opts, + ...stub(`setInterval(() => {}, 1000);`), + timeoutMs: 300, + onSpawn: (c) => (spawned = c), + }); + assert.equal(r.code, 1); + assert.match(r.message, /did not render/); + // The #2000 ownership rule: the child is published before the wait, so the + // timeout path leaves a stoppable handle rather than an orphan. + assert.ok(spawned, "onSpawn was never called"); + assert.notEqual( + spawned.exitCode === null && spawned.signalCode === null, + true, + ); +}); + +test("reports a spawn failure rather than throwing it uncaught", async () => { + const r = await runRenderSmoke({ + ...opts, + command: "/definitely/not/a/binary-2147", + args: [], + }); + assert.equal(r.code, 1); + assert.match(r.message, /failed to spawn/); +}); + +test("scans stderr as well as stdout for the marker", async () => { + // An Ink app writing to stderr is missed entirely by a stdout-only scan, + // which then times out with an empty diagnostic. + const r = await runRenderSmoke({ + ...opts, + ...stub( + `console.error(${JSON.stringify(MARKER)}); setInterval(() => {}, 1000);`, + ), + }); + assert.equal(r.code, 0, `expected pass, got: ${r.message}`); +}); + +// `Number("")` is 0 and `Number("typo")` is NaN, and `??` catches neither +// because both are non-nullish. `setTimeout` fires immediately on either, so an +// un-normalized `surviveMs` resolves the survival check before the child could +// possibly have died — restoring the exact first-paint false green, on a run +// that still prints OK. This is the guard on the guard. +test("normalizeMs rejects the values a bad env var actually produces", () => { + assert.equal(normalizeMs(Number(""), 2000), 2000); + assert.equal(normalizeMs(Number("typo"), 2000), 2000); + assert.equal(normalizeMs(undefined, 2000), 2000); + assert.equal(normalizeMs(-1, 2000), 2000); + assert.equal(normalizeMs(Infinity, 2000), 2000); + assert.equal(normalizeMs("3000", 2000), 2000, "a string is not a duration"); + // 0 is a typo, never "skip the check" — there is no caller who wants that. + assert.equal(normalizeMs(0, 2000), 2000); + // A real value passes through untouched. + assert.equal(normalizeMs(1234, 2000), 1234); +}); + +test("a zero survival window falls back rather than passing instantly", async () => { + // End to end through the real state machine: with surviveMs honored as 0 the + // paint-then-exit stub would settle OK before the child's exit was seen. + const r = await runRenderSmoke({ + ...opts, + ...paintThenExit(1), + surviveMs: Number(""), + }); + assert.equal(r.code, 1, `expected failure, got: ${r.message}`); + // The window named in the message is the normalized one, not the 0 passed in. + assert.match( + r.message, + new RegExp(`${DEFAULTS.surviveMs}ms survival window`), + ); + assert.doesNotMatch(r.message, /\b0ms survival window/); +}); + +// `forbidOutput` is `smoke:tui`'s guard for the acceptance criterion that the +// raw-mode error must not appear in a passing run. It lives here rather than at +// the call site precisely so it can be driven both ways: the real PTY happy +// path only ever exercises the pattern NOT matching, so a regression that broke +// this check would stay green forever. +test("forbidOutput fails a run that would otherwise have passed", async () => { + const r = await runRenderSmoke({ + ...opts, + ...stub( + `console.log(${JSON.stringify(MARKER)}); ` + + `console.error("Raw mode is not supported on the current process.stdin"); ` + + `setInterval(() => {}, 1000);`, + ), + forbidOutput: { + pattern: /Raw mode is not supported/, + reason: "survived but logged the raw-mode error", + }, + }); + assert.equal(r.code, 1, `expected failure, got: ${r.message}`); + assert.match(r.message, /survived but logged the raw-mode error/); + // The offending output is quoted, so the reader sees the evidence. + assert.match(r.message, /Raw mode is not supported/); +}); + +test("forbidOutput leaves a passing run alone when it does not match", async () => { + const r = await runRenderSmoke({ + ...opts, + ...paintThenLive, + forbidOutput: { + pattern: /Raw mode is not supported/, + reason: "should not be reached", + }, + }); + assert.equal(r.code, 0, `expected pass, got: ${r.message}`); + assert.doesNotMatch(r.message, /should not be reached/); +}); + +// A crash reason is more useful than "and it also printed X", which is usually +// a symptom of the same crash rather than a separate finding. +test("forbidOutput does not overwrite an existing failure's diagnostic", async () => { + const r = await runRenderSmoke({ + ...opts, + ...stub( + `console.error("Raw mode is not supported"); process.exitCode = 1;`, + ), + forbidOutput: { + pattern: /Raw mode is not supported/, + reason: "should not replace the crash reason", + }, + }); + assert.equal(r.code, 1); + assert.match(r.message, /before rendering/); + assert.doesNotMatch(r.message, /should not replace the crash reason/); +}); + +test("outputTail drops a leading partial line but keeps short output whole", () => { + assert.equal(outputTail("short", 800), "short"); + // Over the limit: the slice can land mid-escape-sequence, so it starts after + // the first newline instead of leading with a fragment. + const long = "x".repeat(50) + "\nkeep me"; + assert.equal(outputTail(long, 20), "keep me"); + // No newline in the tail at all: nothing to trim to, so return it as sliced. + assert.equal(outputTail("y".repeat(50), 10), "y".repeat(10)); +}); diff --git a/scripts/smoke-tui.mjs b/scripts/smoke-tui.mjs index f5a8cd9d..7f17a288 100644 --- a/scripts/smoke-tui.mjs +++ b/scripts/smoke-tui.mjs @@ -6,13 +6,28 @@ * the Ink app, so the launcher → TUI → core path and `--catalog` loading went * unverified by the launcher smokes. This script launches the built launcher in * `--tui` mode against a temp `--catalog`, waits for the app to render its first - * frame (the "MCP Servers" panel) within a timeout, then sends SIGTERM and - * exits. + * frame (the "MCP Servers" panel), waits again to confirm it is *still running*, + * then sends SIGTERM and exits. * - * It asserts the TUI *boots and renders* without crashing — not full - * interaction (driving an Ink UI deterministically in CI is flaky, so this is - * intentionally a shallow render check). Exits non-zero on a crash-before-render - * or a render timeout. + * It asserts the TUI boots, renders, and keeps running — not full interaction + * (driving an Ink UI deterministically is flaky, so this is intentionally a + * shallow check). Exits non-zero on a crash before or after first paint, or on + * a render timeout. + * + * ## Why it runs under a pseudoterminal (#2147) + * + * It used to spawn the TUI with its stdin on `/dev/null` and settle OK the + * moment the marker appeared. Ink mounts `useInput`, `useInput` needs raw mode, + * and raw mode is a property of the file descriptor — so the TUI painted one + * frame and died ~40ms later with "Raw mode is not supported on the current + * process.stdin". The smoke won that race and reported OK, on every machine, + * TTY or not: what it actually asserted was *first paint*, not survival. + * + * Two changes, and the second is the fix: `scripts/lib/pty.mjs` gives the child + * a real terminal via `script(1)`, and `scripts/lib/render-smoke.mjs` requires + * it to outlive its first frame. Without the survival assertion a future + * harness change reintroduces exactly this false green and nothing notices — + * which is why that assertion lives in a tested module rather than here. * * Expects `clients/launcher/build` and `clients/tui/build` to be built first * (the validate / CI ordering guarantees this). The bundled stdio test server @@ -20,39 +35,72 @@ * `scripts/lib/ensure-test-servers.mjs` for why presence isn't freshness. */ -import { spawn } from "node:child_process"; import { mkdtempSync, existsSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join, resolve } from "node:path"; import { removeSafe } from "./lib/child-cleanup.mjs"; import { ensureTestServers } from "./lib/ensure-test-servers.mjs"; +import { resolvePtyWrapper } from "./lib/pty.mjs"; +import { DEFAULTS, normalizeMs, runRenderSmoke } from "./lib/render-smoke.mjs"; const repoRoot = resolve(import.meta.dirname, ".."); const launcher = join(repoRoot, "clients", "launcher", "build", "index.js"); const RENDER_MARKER = "MCP Servers"; -const TIMEOUT_MS = Number(process.env.SMOKE_TUI_TIMEOUT_MS ?? 15000); +// `??` does not cover the two holes that matter here: an env var set to the +// empty string parses as 0, and a typo as NaN, and `setTimeout` fires +// immediately on both. For SURVIVE_MS that would silently restore the +// first-paint false green on a run that still prints OK, so every duration goes +// through `normalizeMs` — which is also where the defaults live, so this file +// and the module cannot drift on what they are. +const envMs = (name, fallback) => + normalizeMs(Number(process.env[name]), fallback); + +const TIMEOUT_MS = envMs("SMOKE_TUI_TIMEOUT_MS", DEFAULTS.timeoutMs); +// How long past first paint the TUI must still be running. Generous relative to +// the failure it catches (the raw-mode crash landed ~40ms after the marker) — +// the cost of a longer window is only wall-clock on a passing run. +const SURVIVE_MS = envMs("SMOKE_TUI_SURVIVE_MS", DEFAULTS.surviveMs); +const EXIT_GRACE_MS = envMs("SMOKE_TUI_EXIT_GRACE_MS", DEFAULTS.exitGraceMs); +// Ink's own error text when it is asked for raw mode on a non-terminal fd. +const RAW_MODE_ERROR = /Raw mode is not supported/; function fail(message) { console.error(`smoke:tui FAILED — ${message}`); process.exit(1); } -// The Ink TUI requires a real TTY for raw-mode keyboard input. Headless CI has -// none, so the app throws "Raw mode is not supported" on mount and exits before -// it can render its first frame — making this boot/render check inherently a -// LOCAL-only smoke (its own design notes call driving Ink in CI flaky). Skip it -// under CI rather than fail spuriously: the TUI is still built and unit-tested -// there; only this terminal-dependent render check is local-only. -if (process.env.CI) { - console.log( - "smoke:tui SKIPPED — Ink needs a real TTY (raw mode), unavailable in CI; run it locally", - ); +function skip(message) { + console.log(`smoke:tui SKIPPED — ${message}`); process.exit(0); } +// Kept out of GitHub CI by decision, not by capability. The PTY above removes +// the technical blocker this skip used to cite (a headless runner has no TTY), +// but whether this smoke joins CI is a separate call for the maintainers, and +// #2146 is deliberately keeping the other local-only smokes out. Making it +// *valid* stands on its own: it is a local-only gate, which is exactly where a +// false green is least likely to be caught by anything else. +if (process.env.CI) { + skip( + "local-only by decision (see the header); the TUI is built and unit-tested in CI", + ); +} + if (!existsSync(launcher)) { fail(`launcher build not found at ${launcher} — run \`npm run build\` first`); } + +// No `script(1)` (Windows). Running without a PTY is not a weaker check, it is +// a guaranteed failure — so say why rather than report a crash as a defect. +// `node-pty` is the portable answer if this ever needs to run there. +const pty = resolvePtyWrapper(); +if (!pty.ok) { + skip( + `${pty.reason} — the Ink TUI needs raw mode, which requires a real ` + + `terminal fd (see scripts/lib/pty.mjs)`, + ); +} + // Rebuilt on every run — presence is not freshness (#2111). let testServer; try { @@ -76,183 +124,57 @@ writeFileSync( }), ); -const child = spawn( - process.execPath, - [launcher, "--tui", "--catalog", catalogPath], - { - cwd: repoRoot, - // Redirect HOME so the TUI's storage never touches the real ~/.mcp-inspector. - // Pin MCP_OAUTH_CALLBACK_URL="" (empty reads as unset) so an ambient - // non-loopback value can't crash the TUI before render via the loopback - // callback guard — same class smoke-cli.mjs's SMOKE_BASE_ENV neutralizes. - env: { - ...process.env, - MCP_OAUTH_CALLBACK_URL: "", - HOME: work, - USERPROFILE: work, +const { command, args } = pty.wrap({ + command: process.execPath, + args: [launcher, "--tui", "--catalog", catalogPath], +}); + +let result; +try { + result = await runRenderSmoke({ + command, + args, + marker: RENDER_MARKER, + timeoutMs: TIMEOUT_MS, + surviveMs: SURVIVE_MS, + exitGraceMs: EXIT_GRACE_MS, + spawnOptions: { + cwd: repoRoot, + // Redirect HOME so the TUI's storage never touches the real + // ~/.mcp-inspector. Pin MCP_OAUTH_CALLBACK_URL="" (empty reads as unset) + // so an ambient non-loopback value can't crash the TUI before render via + // the loopback callback guard — same class smoke-cli.mjs's + // SMOKE_BASE_ENV neutralizes. + env: { + ...process.env, + MCP_OAUTH_CALLBACK_URL: "", + HOME: work, + USERPROFILE: work, + }, }, - stdio: ["ignore", "pipe", "pipe"], - }, -); - -let output = ""; -let settled = false; -let childExited = false; -let childClosed = false; - -// How long to let the TUI wind down after SIGTERM before escalating to SIGKILL, -// and then before giving up on the close event entirely. -const EXIT_GRACE_MS = Number(process.env.SMOKE_TUI_EXIT_GRACE_MS ?? 5000); -// Once the child itself is gone, only the pipe drain remains, so the wait drops -// to this. `close` is bounded by whoever holds the stdio pipes, which can -// outlive the direct child: any descendant that inherited them keeps it pending. -// The TUI spawns nothing at boot today (it does not auto-connect), so this -// never fires — but that reason lives outside this file, so cap it here rather -// than inherit an unrelated process's lifetime if that ever changes. -// -// The cost of capping: on that path we remove the work dir while a descendant -// holding HOME=work may still be writing to it — the #1801 race, re-entered -// deliberately. That is only acceptable because cleanup() warns instead of -// throwing, so the worst case is a warning plus a leaked temp dir in tmpdir, -// never a red smoke. The two are load-bearing for each other. -const DRAIN_MS = 500; - -function cleanup() { + // Belt and braces for the acceptance criterion this fix is measured + // against. Survival already implies it today, since the raw-mode throw is + // fatal — but if Ink ever degrades it to a warning, a TUI with no keyboard + // input would survive happily, and this smoke is the only thing watching. + forbidOutput: { + pattern: RAW_MODE_ERROR, + reason: + `TUI rendered and survived, but still logged the raw-mode error — ` + + `the pseudoterminal did not take effect (flavor: ${pty.flavor})`, + }, + warn: (m) => console.warn(m.replace(/^render-smoke/, "smoke:tui")), + }); +} finally { // Never turn a passing smoke into a failure over a leftover temp dir; the OS - // reclaims tmpdir anyway. On the normal path a warning should not fire, since - // removal waits for the child's `close` (see done()) — but the two give-up - // branches there call finish() without it, and a process may still hold the - // dir, so it is expected rather than anomalous on those. + // reclaims tmpdir anyway. runRenderSmoke resolves only after the child's + // `close`, so on the normal path the #1801 ENOTEMPTY race is already closed + // and a warning here should not fire — its give-up branches resolve without + // that guarantee, so it is expected rather than anomalous on those. removeSafe(work, { label: "smoke:tui" }); } -// Tail of the child's output, for quoting in a diagnostic. Slicing an Ink -// stream can land mid-CSI-sequence, so drop through the first newline rather -// than lead with an escape-code fragment that mangles the line after it. -function outputTail(limit = 800) { - const tail = output.slice(-limit); - const nl = tail.indexOf("\n"); - return output.length > limit && nl !== -1 ? tail.slice(nl + 1) : tail; +if (result.code === 0) { + console.log(`smoke:tui OK — ${result.message}`); + process.exit(0); } - -// `message` may be a string or a thunk. Callers that quote the child's output -// pass a thunk so it is rendered here — after the wait below, by which point -// `close` guarantees stdout/stderr have been fully drained. Building the string -// at call time instead can truncate the crash reason that matters most. -let finished = false; -function finish(code, message) { - if (finished) return; - finished = true; - cleanup(); - const text = typeof message === "function" ? message() : message; - if (code === 0) { - console.log(`smoke:tui OK — ${text}`); - } else { - console.error(`smoke:tui FAILED — ${text}`); - } - process.exit(code); -} - -function done(code, message) { - if (settled) return; - settled = true; - clearTimeout(timer); - if (childClosed) { - finish(code, message); - return; - } - // Wait for the child to actually exit before removing the work dir (#1801). - // The dir doubles as the TUI's HOME, so the Ink process is still writing into - // it when we signal; an rmSync racing those writes fails with ENOTEMPTY when - // a file lands after a directory has been read but before it is removed. - // - // Wait on `close`, not `exit`: a *spawn failure* emits `error` + `close` and - // never `exit`, so an `exit` wait would hang here until the give-up timer and - // then blame a SIGTERM the child never received. `close` also guarantees the - // stdio pipes are drained, which is what makes the thunked messages complete. - // - // This deliberately does NOT use lib/child-cleanup.mjs's `stopChild()` (which - // pack-and-verify.mjs uses against the same race): that helper resolves on the - // FIRST of exit/close, whereas this wait is entangled with the drain phase — - // it must keep waiting for `close` after `exit`, on a shorter re-armed - // deadline, so the quoted output is complete. Only `removeSafe()` is shared. - const forceKill = setTimeout(() => child.kill("SIGKILL"), EXIT_GRACE_MS); - - // Deadline for the wait. Re-armed shorter once the child is gone, since from - // that point we are only draining pipes. Each message describes what was - // actually being waited on — the timer can fire for a child that already - // exited and was never signalled, so it must not claim otherwise. - let deadline; - const armDeadline = (ms, warning) => { - clearTimeout(deadline); - deadline = setTimeout(() => { - console.warn(`smoke:tui — ${warning}`); - finish(code, message); - }, ms); - }; - armDeadline( - EXIT_GRACE_MS * 2, - `TUI did not close its output streams within ${EXIT_GRACE_MS * 2}ms; cleaning up anyway`, - ); - - const drainOnly = () => { - clearTimeout(forceKill); - armDeadline( - DRAIN_MS, - `TUI exited but held its output streams open for ${DRAIN_MS}ms (a descendant may have inherited them); cleaning up anyway`, - ); - }; - if (childExited) drainOnly(); - else child.once("exit", drainOnly); - - child.once("close", () => { - clearTimeout(forceKill); - clearTimeout(deadline); - finish(code, message); - }); - // Only signal a child that is still running: on the crash-before-render and - // spawn-failure paths there is nothing left to signal, and we are here purely - // to wait out the remaining `close`. - if (child.exitCode === null && child.signalCode === null && !child.killed) { - child.kill("SIGTERM"); - } -} - -function onData(chunk) { - output += chunk.toString(); - if (output.includes(RENDER_MARKER)) { - done(0, `rendered "${RENDER_MARKER}" panel from --catalog within timeout`); - } -} - -child.stdout.on("data", onData); -child.stderr.on("data", onData); - -// Registered before done()'s own one-shot listener, so this always runs first — -// done() can trust `childClosed` even when called from inside a close handler. -child.on("close", () => { - childClosed = true; -}); - -child.on("exit", (code) => { - childExited = true; - if (settled) return; - // Exiting before the render marker appeared is a failure (crash on boot). - done( - 1, - () => - `TUI exited (code ${code}) before rendering "${RENDER_MARKER}"\n${outputTail()}`, - ); -}); - -child.on("error", (err) => { - done(1, `failed to spawn TUI: ${err.message}`); -}); - -const timer = setTimeout(() => { - done( - 1, - () => - `TUI did not render "${RENDER_MARKER}" within ${TIMEOUT_MS}ms\n${outputTail()}`, - ); -}, TIMEOUT_MS); +fail(result.message);